Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MS5611 Driver Update #2 #53

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

MS5611 Driver Update #2 #53

wants to merge 13 commits into from

Conversation

FolaFatola
Copy link

Description

What was completed, changed, or updated?
Update of MS5611 Driver. Function return types changed from bool to float with FLT_MAX used to signify errors. Communication_success_ internal variable removed and replaced with other internal variables.

Temperature, pressure and altitude values were printed to the UART. For the altitude, the barometer was raised a few meters and measurements were observed. For temperature, values were compared to local thermometer. The pressure was compared to the pressure of the city I was in (googled).

Testing

image

What manual tests were used to validate the code?
Screen Shot 2024-05-21 at 11 44 12 PM


What unit tests were used to validate the code?

Reminders

  • Add reviewers to the PR

  • Mention the PR in the appropriate discord channel

@FolaFatola FolaFatola changed the title Drivers/altimeter MS5611 Driver Update #2 May 22, 2024
Copy link
Contributor

@HardyYu HardyYu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done Fola! It looks so good. I still left some comments here but they are all minor, and they will not change the fact that this code will work.

HAL_GPIO_WritePin(cs_port_, cs_pin_, GPIO_PIN_SET);

// construct 16-bit calibration coefficient.
uint16_t prom_element = ((rx_data[1] << 8) | (rx_data[2])) & 0x0000FFFF;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This mask 0x0000FFFF has a size of 32 bits which is confusing here since prom_element has only a size of uint16_t.


uint8_t tx_data = conversion_command;
uint8_t rx_data;
uint8_t tx_data_2[4] = {ADC_READ, 0xFF, 0xFF, 0xFF};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this can be a const. Same with the other tx_data.

HAL_GPIO_WritePin(cs_port_, cs_pin_, GPIO_PIN_RESET);

// send the digital pressure or temperature with the command.
HAL_StatusTypeDef spi_status_ = HAL_SPI_TransmitReceive_IT(spi_handle_, &tx_data, &rx_data, data_size);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, it was me in the previous PR saying maybe we could use interrupt-based data transceiving in the future, but I actually meant using DMA. And here I think it makes sense to use normal polling since you are reading the transmitting at the same time. I thought if you call the hal function ended with _IT, you are also triggering an ISR callback function, but you also don't need to use it here, so there is not a big difference between using interrupt base or pulling based transceiving.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants