Added altimeter .hpp and .cpp files #47

FolaFatola · 2024-04-15T05:12:34Z

Description

What was completed, changed, or updated?
Added altimeter driver .hpp and .cpp files.

Reads temperature and pressure and calculates height in meters.

Why was this done (if applicable)?

Testing

What manual tests were used to validate the code?
Printed values to terminal using UART.

What unit tests were used to validate the code?

Documentation

Milestone number and name:
M4

Link to Asana task:
https://app.asana.com/0/1203458353737758/1204645027312243

Link to Confluence documentation:

HardyYu

Good stuff! Can't wait to see it get used in ZP

HardyYu · 2024-04-15T23:16:30Z

Altimeter/Inc/altimeter.hpp

+
+#define TIMEOUT 1 << 31
+
+typedef struct cal_coeffs{


could you change the struct name according to the ZP style standards? https://uwarg-docs.atlassian.net/wiki/spaces/ZP/pages/2268692486/Style+Standards

HardyYu · 2024-04-16T02:01:21Z

Altimeter/Inc/altimeter.hpp

+ *
+ * @return none
+ */
+ AltDevice(GPIO_TypeDef *ps_port, uint16_t ps_pin);


In my opinion, a driver code could be a lot easier to use if we only initiate the device once with the SPI, cs, ps line bundled with this driver code so that the user of your driver doesn't have to know the SPI it is using every time they call get_altitude function. That means I think SPI_HandleTypeDef and GPIO_TypeDef they should be the private variable of this class.

HardyYu · 2024-04-16T02:12:05Z

Altimeter/Src/altimeter.cpp

+ HAL_GPIO_WritePin(cs_port, cs_pin, GPIO_PIN_RESET);
+
+ /* send the digital pressure or temperature with the command */
+ HAL_SPI_TransmitReceive(spi_handle, digital_pres_temp, receive_buffer, 1, TIMEOUT);


I think this function could be changed to an interrupt based version of SPI receive in the next revision, so when the worst situation happened, we are not waiting for this timeout forever. Every return value from the communication function call to an external device should be checked. The failed communication should be handled explicitly, and inform to the function caller.

HardyYu · 2024-04-16T02:13:53Z

Altimeter/Src/altimeter.cpp

+ /* at least 8.22 ms delay required for ADC conversion.*/
+ osDelay(9);
+
+ uint8_t tx_buffer[4] = {ADC_READ, 0xFF, 0xFF, 0xFF};


this variable is never changed, please add const and capitalize this variable

HardyYu · 2024-04-16T02:16:52Z

Altimeter/Src/altimeter.cpp

+ HAL_GPIO_WritePin(cs_port, cs_pin, GPIO_PIN_SET);
+
+ //get 24-bit uncompensated pressure or temperature value value.
+ uint32_t uncompensated_value = (rx_buffer[1] << 16 | rx_buffer[2] << 8 | rx_buffer[3]);


also do a & 0x00FFFFFF at the end?

HardyYu · 2024-04-16T02:20:07Z

Altimeter/Src/altimeter.cpp

+
+ }
+
+ float pressure = (digital_pressure * (sens / 2097152.0f) - off) / 32768.0f;


A lot of magic numbers here in this function, it will be the best if each of them can be assigned with a name. No need for the small whole numbers such as 2, 5.

HardyYu · 2024-04-16T02:20:54Z

Altimeter/Src/altimeter.cpp

+ float temp_lapse_rate = 0.0065f;
+ float exp_gmrl = 5.2558;
+
+ float reference_pressure = 101325.0f; /* in Pa */


yeah like what you are doing here is awesome

antholuo

Overall looks pretty good and solid, very pleased with what we have so far.

Some formatting changes requested (take a look at WARG's clang-format script).

Also wanted to make sure we have clear variable names, and are managing our classes / structs / data properly in order to make future-us lives easier!

edit: Also try and include full test-methodology and screenshots/photos in the PR! You want to make it as easy as possible for someone else to re-create your test and verify results, be it now or 5 months in the future

antholuo · 2024-04-16T01:29:30Z

Altimeter/Inc/altimeter.hpp

+#define PROM_READ_ADDRESS_0 0xA0
+#define PROM_READ_ADDRESS_1 0xA2
+#define PROM_READ_ADDRESS_2 0xA4
+#define PROM_READ_ADDRESS_3 0xA6
+#define PROM_READ_ADDRESS_4 0xA8
+#define PROM_READ_ADDRESS_5 0xAA
+#define PROM_READ_ADDRESS_6 0xAC
+#define PROM_READ_ADDRESS_7 0xAE
+
+
+/*digital pressure/temperature commands at different sampling rates*/
+#define CONVERT_D1_OSR_256 0x40
+#define CONVERT_D1_OSR_512 0x42
+#define CONVERT_D1_OSR_1024 0x44
+#define CONVERT_D1_OSR_2048 0x46
+#define CONVERT_D1_OSR_4096 0x48
+#define CONVERT_D2_OSR_256 0x50
+#define CONVERT_D2_OSR_512 0x52
+#define CONVERT_D2_OSR_1024 0x54
+#define CONVERT_D2_OSR_2048 0x56
+#define CONVERT_D2_OSR_4096 0x58
+
+#define ADC_READ 0x0
+
+#define TIMEOUT 1 << 31


Unless you have a very specific reason to use #define, I would strongly recommend using constexpr.

Not only is it safer, you can also then move your values into the private field of your device Class, making other functions safer // cleaner!

Depending on the compiler, you might also see performance benefits from using constexpr, although it depends on your optimization level and chip architecture.

antholuo · 2024-04-16T01:30:54Z

Altimeter/Inc/altimeter.hpp

+
+#define TIMEOUT 1 << 31
+
+typedef struct cal_coeffs{


I would prefer a longer, more descriptive name here. What are these cal coeffs for? What is a cal? etc.

If it's possible to, I would recommend typing out baro_calibration_coefficients, or even ms5611_calibration_coeffs.

I also want to ask why this needs to be a stand-alone struct and not part of the class?

antholuo · 2024-04-16T01:32:05Z

Altimeter/Inc/altimeter.hpp

+ uint16_t sens;
+ uint16_t off;
+ uint16_t tcs;
+ uint16_t tco;
+ uint16_t t_ref;
+ uint16_t temp_sens;


The same comment about descriptive names from earlier applies here as well.

Its hard for me to read this and understand what tcs or t_ref or tco mean? Temperature controlled oscillator? Timer clock speed? etc...

I can make a guess for sure, but it's best not to let the reader or future editors make guesses and assumptions when reading code that's flying.

antholuo · 2024-04-16T01:32:25Z

Altimeter/Inc/altimeter.hpp

+ uint16_t tco;
+ uint16_t t_ref;
+ uint16_t temp_sens;
+}cal_coeffs;


Suggested change

}cal_coeffs;

} cal_coeffs;

This one is really just me being nitpicky, i'm sorry :(

antholuo · 2024-04-16T01:33:18Z

Altimeter/Inc/altimeter.hpp

+
+
+/* To get temperature/pressure/altitude values, call (step 1) AltDevice, then (step 2) AltInit once.
+ * Then before getting (step 4) pressure/temp/altitude, (step 3) calculateTempPres should be called beforehand.


Why does all of this need separate steps? Should our init() handle all of these calls so that a user can initialize the barometer and then use it?

antholuo · 2024-04-16T02:14:14Z

Altimeter/Src/altimeter.cpp

+uint32_t AltDevice::uncompensatedPressureTemperature(SPI_HandleTypeDef *spi_handle, GPIO_TypeDef *cs_port, uint16_t cs_pin, uint8_t conversion_command){
+
+ uint8_t digital_pres_temp[1] = {conversion_command};
+ uint8_t receive_buffer[1];


same here , why are we making an array of depth 1?

antholuo · 2024-04-16T02:23:11Z

Altimeter/Src/altimeter.cpp

+ float off = coeffs_.off * 65536.0f + (coeffs_.tco * dt) / 128.0f;
+ float sens = coeffs_.sens * 32768.0f + (coeffs_.tcs * dt) / 256.0f;


Descriptive naming here too please!

antholuo · 2024-04-16T02:23:31Z

Altimeter/Src/altimeter.cpp

+ float sens = coeffs_.sens * 32768.0f + (coeffs_.tcs * dt) / 256.0f;
+
+ /*Second order temperature compensation */
+ if(temp < 2000){


Suggested change

if(temp < 2000){

if (temp < 2000) {

Take a look at warg's clang-format if you haven't already, it should take care of stuff like this

antholuo · 2024-04-16T02:24:01Z

Altimeter/Src/altimeter.cpp

+ float pressure = (digital_pressure * (sens / 2097152.0f) - off) / 32768.0f;
+ pressure /= 100.0f;
+ temp /= 100.0f;
+
+ pres_ = pressure;
+ temp_ = temp;


Comments for magic numbers would be helpful.

In general, we try and avoid magic numbers and instead use consts with clear names!

antholuo · 2024-04-16T02:25:00Z

Altimeter/Src/altimeter.cpp

+
+
+float AltDevice::getAltitude(){
+ height_ = getAltAboveSeaLvl();


is height_ a private var? Why are we modifying a private var here but not in the above function?

…nce with spi handler and cs/ps port and pins

…1 class, and more descriptive naming for MS5611 calibration coefficients

…iption formatting on calculateTempPres() function

…led in getPressure() and getTempemrautre() functions

…ructor

…unction

Added altimeter .hpp and .cpp files

9698f57

FolaFatola requested review from antholuo, ayoungeun, DerekTang04 and HardyYu April 15, 2024 05:12

HardyYu reviewed Apr 16, 2024

View reviewed changes

antholuo requested changes Apr 16, 2024

View reviewed changes

FolaFatola added 11 commits April 18, 2024 20:08

Struct name changed to meet ZP style standards and device initiated o…

428b4bc

…nce with spi handler and cs/ps port and pins

defined magic numbers

482d73a

#defines changed to constexpr

a96edd3

cal_coeff struct members changed to private member variables of MS561…

f54a5ce

…1 class, and more descriptive naming for MS5611 calibration coefficients

Changed ms5611 altitude names to be morme distinct and adjusted descr…

6977b63

…iption formatting on calculateTempPres() function

Temperature and pressure calculation function name changed and is cal…

15bf1a9

…led in getPressure() and getTempemrautre() functions

Digital pressure/temperature read function name modified

7d60810

More descriptive tx and rx buffer names, and Init code moved to const…

5d39043

…ructor

getAltitudeAboveSeaLevel() function variables now const

314a711

Modified functions to allow for communication error checking

9768b64

Name changes made to variables in calculateTemperatureAndPressure() f…

27739b0

…unction

FolaFatola mentioned this pull request Apr 29, 2024

MS5611 Driver Update #49

Closed

DerekTang04 closed this May 6, 2024

DerekTang04 deleted the drivers/altimeter branch May 6, 2024 22:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added altimeter .hpp and .cpp files #47

Added altimeter .hpp and .cpp files #47

FolaFatola commented Apr 15, 2024

HardyYu left a comment

HardyYu Apr 15, 2024

HardyYu Apr 16, 2024

HardyYu Apr 16, 2024

FolaFatola Apr 18, 2024

HardyYu Apr 16, 2024

HardyYu Apr 16, 2024

HardyYu Apr 16, 2024

FolaFatola Apr 22, 2024

HardyYu Apr 16, 2024

antholuo left a comment •

edited

Loading

antholuo Apr 16, 2024

antholuo Apr 16, 2024

antholuo Apr 16, 2024

antholuo Apr 16, 2024

antholuo Apr 16, 2024

antholuo Apr 16, 2024

antholuo Apr 16, 2024

antholuo Apr 16, 2024

antholuo Apr 16, 2024

antholuo Apr 16, 2024


		}

		float pressure = (digital_pressure * (sens / 2097152.0f) - off) / 32768.0f;



		/* To get temperature/pressure/altitude values, call (step 1) AltDevice, then (step 2) AltInit once.
		* Then before getting (step 4) pressure/temp/altitude, (step 3) calculateTempPres should be called beforehand.

		float off = coeffs_.off * 65536.0f + (coeffs_.tco * dt) / 128.0f;
		float sens = coeffs_.sens * 32768.0f + (coeffs_.tcs * dt) / 256.0f;



		float AltDevice::getAltitude(){
		height_ = getAltAboveSeaLvl();

Added altimeter .hpp and .cpp files #47

Added altimeter .hpp and .cpp files #47

Conversation

FolaFatola commented Apr 15, 2024

Description

Testing

Documentation

HardyYu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antholuo left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antholuo left a comment •

edited

Loading