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

Added altimeter .hpp and .cpp files #47

Closed
wants to merge 12 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
166 changes: 166 additions & 0 deletions Altimeter/Inc/altimeter.hpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,166 @@
#ifndef INC_ALTIMETER_HPP_
#define INC_ALTIMETER_HPP_

#include "stm32l5xx_hal.h"
#include "cmsis_os.h"
#include <cstdint>
#include <cmath>


#define RESET 0x1E

/*PROM Addresses */
#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
Copy link
Member

Choose a reason for hiding this comment

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

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.


typedef struct cal_coeffs{
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

uint16_t sens;
uint16_t off;
uint16_t tcs;
uint16_t tco;
uint16_t t_ref;
uint16_t temp_sens;
Copy link
Member

Choose a reason for hiding this comment

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

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.

}cal_coeffs;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
}cal_coeffs;
} cal_coeffs;

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




/* 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.
Copy link
Member

Choose a reason for hiding this comment

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

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?

* */

class AltDevice{
public:
/**
* Constructor for the AltDevice class
*
* @param ps_port -> protocol select port
* @param ps_pin -> protocol select pin
*
* @return none
*/
AltDevice(GPIO_TypeDef *ps_port, uint16_t ps_pin);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.


/**
* This function sends the reset command, populates the cal_coeffs
* struct members with the calibration coefficients in the sensor PROM,
* and gets the current elevation above sea level.
*
* @param ps_port -> protocol select port
* @param ps_pin -> protocol select pin
* @param spi_handle -> hspix handle
*
* @return none
*/
void altInit(SPI_HandleTypeDef *spi_handle, GPIO_TypeDef *cs_port, uint16_t cs_pin);
Copy link
Member

Choose a reason for hiding this comment

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

I don't think your @params match with your variables here....

Same comment about naming variables clearly from above also applies to most of the functions we have here.


/**
* Calculates datasheet variable values needed for temperature and pressure,
* and calculates temperature and pressure. It
Copy link
Member

Choose a reason for hiding this comment

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

I don't know if this docstring is complete

*
* @param ps_port -> protocol select port
* @param ps_pin -> protocol select pin
* @param spi_handle -> hspix handle
*
* @return none
*/
void calculateTempPres(SPI_HandleTypeDef *spi_handle, GPIO_TypeDef *cs_port, uint16_t cs_pin);
Copy link
Member

Choose a reason for hiding this comment

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

This is also a nit, but your @params should be in the same order that the variables appear in your function.

Copy link
Member

Choose a reason for hiding this comment

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

Another thought, calculateTempPres might not be the best function name. I'm not sure what it does? Calculate temporary pressure? Calculate temperature presence? etc....

Again, I can guess, but it's only a few more characters to write out the function



/**
* @return the current pressure value
*/
float getPressure();

/**
* @return the current temperature value
*/
float getTemperature();

/**
* @return the current altitude above sea level.
*/
float getAltAboveSeaLvl();

/**
* @return the current altitude above zero point.
*/
float getAltitude();
Copy link
Member

Choose a reason for hiding this comment

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

I think these can be changed to:

getAltitudeASL and getAltitudeAGL

or:

getAltitudeAboveSeaLevel and getAltitudeAboveGroundLevel.

It's not great if you have two very similar functions that are named super differently



private:
cal_coeffs coeffs_;
Copy link
Member

Choose a reason for hiding this comment

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

If no other device uses cal_coeffs as a struct, does the struct need to be defined globally or can it just be part of the class?

Something like this: https://stackoverflow.com/questions/2543205/define-a-struct-inside-a-class-in-c

float base_elev_;
float height_;
float temp_;
float pres_;

/**
* Sends the reset command to the barometer.
Copy link
Member

Choose a reason for hiding this comment

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

It would not be a bad idea to (briefly) include what that reset command is and how it functions. Take a look at this comment: https://github.com/PX4/PX4-Autopilot/blob/94d496605a316d3dc0be795c710eaa6860349960/src/drivers/barometer/ms5611/MS5611.hpp#L100

*
* @param ps_port -> protocol select port
* @param ps_pin -> protocol select pin
* @param spi_handle -> hspix handle
*
* @return none
*/
void reset(SPI_HandleTypeDef *spi_handle, GPIO_TypeDef *cs_port, uint16_t cs_pin);

/**
* Populates the struct with calibration coefficients.
Copy link
Member

Choose a reason for hiding this comment

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

which struct (?)

*
* @param ps_port -> protocol select port
* @param ps_pin -> protocol select pin
* @param spi_handle -> hspix handle
*
* @return none
*/
void getConvCoeffs(SPI_HandleTypeDef *spi_handle, GPIO_TypeDef *cs_port, uint16_t cs_pin);

/**
* Reads user-specified calibration coefficient from the PROM
*
* @param ps_port -> protocol select port
* @param ps_pin -> protocol select pin
* @param spi_handle -> hspix handle
*
* @return calibration data
*/
uint16_t promRead(SPI_HandleTypeDef *spi_handle, GPIO_TypeDef *cs_port, uint16_t cs_pin, uint8_t address);

/**
* Reads uncompenssated temperature/pressure value from altimeter.
*
* @param ps_port -> protocol select port
* @param ps_pin -> protocol select pin
* @param spi_handle -> hspix handle
*
* @return uncompensated pressure/temperature value
*/
uint32_t uncompensatedPressureTemperature(SPI_HandleTypeDef *spi_handle, GPIO_TypeDef *cs_port, uint16_t cs_pin, uint8_t conversion_command);
Copy link
Member

Choose a reason for hiding this comment

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

Our functions should generally follow a item_action_modifier layout or action item modifier layout, so in this case PressureTemperatureReadUncompensated or readPressureTemperatureUncompensated.

It's not super strict, but the end goal is to make sure that a reader is able to discern the overall function or purpose without having to look at docstrings.

};


#endif /* INC_ALTIMETER_HPP_ */
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#endif /* INC_ALTIMETER_HPP_ */
#endif /* INC_ALTIMETER_HPP_ */

Always remember to newline @ EOF!

166 changes: 166 additions & 0 deletions Altimeter/Src/altimeter.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,166 @@
#include <altimeter.hpp>


AltDevice::AltDevice(GPIO_TypeDef *ps_port, uint16_t ps_pin){
HAL_GPIO_WritePin(ps_port, ps_pin, GPIO_PIN_RESET);
}

void AltDevice::altInit(SPI_HandleTypeDef *spi_handle, GPIO_TypeDef *cs_port, uint16_t cs_pin){
Copy link
Member

Choose a reason for hiding this comment

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

in this case, just using Init is fine. We already know what device it will be (ms5611), so init() is a fairly reasonable function.

reset(spi_handle, cs_port, cs_pin);
getConvCoeffs(spi_handle, cs_port, cs_pin);

/*Get the average of 100 readings*/
Copy link
Member

Choose a reason for hiding this comment

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

This needs more explanation. Is it for calibration? etc etc.

Do we have a rough estimate of how long this takes? Does the device need to be still while this happens? etc


float reading_sum = 0.0f;
for(int i = 0; i < 100; i++){
Copy link
Member

Choose a reason for hiding this comment

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

prefer ++i (it's faster, marginally)

calculateTempPres(spi_handle, cs_port, cs_pin);
Copy link
Member

Choose a reason for hiding this comment

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

genuine question, if this is a class, do we need to keep passing in the spi / cs handles or is it possible to save them somewhere as private members?

Copy link
Author

Choose a reason for hiding this comment

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

No. I've made the adjustments. You'll see them in the upcoming pr.

reading_sum += getAltAboveSeaLvl();
}

base_elev_ = reading_sum / 100.0f;
}

void AltDevice::reset(SPI_HandleTypeDef *spi_handle, GPIO_TypeDef *cs_port, uint16_t cs_pin){

HAL_GPIO_WritePin(cs_port, cs_pin, GPIO_PIN_SET);
HAL_GPIO_WritePin(cs_port, cs_pin, GPIO_PIN_RESET);

uint8_t send_reset[3] = {RESET, 0xFF, 0xFF};
uint8_t receive_data[3];

HAL_SPI_TransmitReceive(spi_handle, send_reset, receive_data, 3, TIMEOUT);
osDelay(3);

HAL_GPIO_WritePin(cs_port, cs_pin, GPIO_PIN_SET);
}

void AltDevice::getConvCoeffs(SPI_HandleTypeDef *spi_handle, GPIO_TypeDef *cs_port, uint16_t cs_pin){
coeffs_.sens = promRead(spi_handle, cs_port, cs_pin, PROM_READ_ADDRESS_1);
coeffs_.off = promRead(spi_handle, cs_port, cs_pin, PROM_READ_ADDRESS_2);
coeffs_.tcs = promRead(spi_handle, cs_port, cs_pin, PROM_READ_ADDRESS_3);
coeffs_.tco = promRead(spi_handle, cs_port, cs_pin, PROM_READ_ADDRESS_4);
coeffs_.t_ref = promRead(spi_handle, cs_port, cs_pin, PROM_READ_ADDRESS_5);
coeffs_.temp_sens = promRead(spi_handle, cs_port, cs_pin, PROM_READ_ADDRESS_6);
}


uint16_t AltDevice::promRead(SPI_HandleTypeDef *spi_handle, GPIO_TypeDef *cs_port, uint16_t cs_pin, uint8_t address){
HAL_GPIO_WritePin(cs_port, cs_pin, GPIO_PIN_SET);
HAL_GPIO_WritePin(cs_port, cs_pin, GPIO_PIN_RESET);

uint8_t send_read_command[3] = {address, 0xFF, 0xFF};
Copy link
Member

Choose a reason for hiding this comment

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

if promRead is used often, it would be faster to put this variable as a static const (most compilers will optimize it a bit then). Depends on where your program ends up placing this function in memory.

Copy link
Member

Choose a reason for hiding this comment

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

also - just in the vein of "making things clear", "send_read_command" doesn't make much sense(?) "read_command" or "spi_tx_data" might be better options

uint8_t coefficient_val[3];

HAL_SPI_TransmitReceive(spi_handle, send_read_command, coefficient_val, 3, TIMEOUT);
Copy link
Member

Choose a reason for hiding this comment

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

I say "send_read_command" might not be great because I was reading this function and had no clue what "send_read_command" was doing in this section.

I would prefer seeing HAL_SPI_TransmitReceive(spi_handle, tx_data, rx_data, data_size, TIMEOUT)


HAL_GPIO_WritePin(cs_port, cs_pin, GPIO_PIN_SET);

//Grab conversion coefficients.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
//Grab conversion coefficients.
// Grab conversion coefficients.

uint16_t conversion_coefficient = (coefficient_val[1] << 8) | (coefficient_val[2]);

return conversion_coefficient;
}



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};
Copy link
Member

Choose a reason for hiding this comment

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

why do we need curly braces {}

also...why does this need to be a uint8_t array of 1 depth

uint8_t receive_buffer[1];
Copy link
Member

Choose a reason for hiding this comment

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

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


HAL_GPIO_WritePin(cs_port, cs_pin, GPIO_PIN_SET);
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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

Okay

HAL_GPIO_WritePin(cs_port, cs_pin, GPIO_PIN_SET);

/* at least 8.22 ms delay required for ADC conversion.*/
osDelay(9);

uint8_t tx_buffer[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.

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

uint8_t rx_buffer[4];

HAL_GPIO_WritePin(cs_port, cs_pin, GPIO_PIN_RESET);

/*Start adc conversion to get value */
HAL_SPI_TransmitReceive(spi_handle, tx_buffer, rx_buffer, 4, TIMEOUT);

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]);
Copy link
Contributor

Choose a reason for hiding this comment

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

also do a & 0x00FFFFFF at the end?


return uncompensated_value;
}


void AltDevice::calculateTempPres(SPI_HandleTypeDef *spi_handle, GPIO_TypeDef *cs_port, uint16_t cs_pin){


uint32_t digital_temperature = uncompensatedPressureTemperature(spi_handle, cs_port, cs_pin, CONVERT_D2_OSR_256);
uint32_t digital_pressure = uncompensatedPressureTemperature(spi_handle, cs_port, cs_pin, CONVERT_D1_OSR_256);

float dt = digital_temperature - (float)coeffs_.t_ref * 256.0f;
float temp = 2000 + dt * (float) coeffs_.temp_sens / pow(2, 23);


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

Choose a reason for hiding this comment

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

Descriptive naming here too please!


/*Second order temperature compensation */
if(temp < 2000){
Copy link
Member

Choose a reason for hiding this comment

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

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

float t2 = (dt*dt) / 2147483648.0f;
float off2 = 5.0f * pow((temp - 2000), 2) / 2.0f;
float sens2 = 5.0f * pow((temp - 2000), 2) / 4.0f;


if(temp < 1500){
off2 = off2 + 7 * pow((temp + 1500), 2);
sens2 = sens2 + 11 * pow((temp + 1500), 2) / 2;
}

temp = temp - t2;
off = off - off2;
sens = sens - sens2;

}

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

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

Done

pressure /= 100.0f;
temp /= 100.0f;

pres_ = pressure;
temp_ = temp;
Copy link
Member

Choose a reason for hiding this comment

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

Comments for magic numbers would be helpful.

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

}


float AltDevice::getPressure(){
return pres_;
}

float AltDevice::getTemperature(){
return temp_;
}

float AltDevice::getAltAboveSeaLvl(){

float reference_temp = 288.15f; /* Ref temperature in Kelvins */
float temp_lapse_rate = 0.0065f;
float exp_gmrl = 5.2558;

float reference_pressure = 101325.0f; /* in Pa */
Copy link
Contributor

Choose a reason for hiding this comment

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

yeah like what you are doing here is awesome

float current_pressure = getPressure() * 100.0f;
float exponent = (log(current_pressure) - log(reference_pressure)) / exp_gmrl;

float height = reference_temp/temp_lapse_rate * (1 - pow(M_E, exponent));

return height;
}


float AltDevice::getAltitude(){
height_ = getAltAboveSeaLvl();
Copy link
Member

Choose a reason for hiding this comment

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

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

return {height_ - base_elev_};
}

Loading