feat(vacuum-module): more sensor hardware + sensor read functionality #543
Conversation
stm32-modules/include/vacuum-module/vacuum-module/MPRLL0025PA00001A.hpp
Outdated
Show resolved
Hide resolved
stm32-modules/include/vacuum-module/vacuum-module/MPRLL0025PA00001A.hpp
Outdated
Show resolved
Hide resolved
sfoster1
left a comment
sfoster1
left a comment
There was a problem hiding this comment.
Confused by those min and max reading numbers - those need 24 or so bits, so either the data type is wrong or the numbers are wrong
stm32-modules/include/vacuum-module/vacuum-module/MPRLL0025PA00001A.hpp
Outdated
Show resolved
Hide resolved
stm32-modules/include/vacuum-module/vacuum-module/MPRLL0025PA00001A.hpp
Outdated
Show resolved
Hide resolved
stm32-modules/include/vacuum-module/vacuum-module/MPRLL0025PA00001A.hpp
Outdated
Show resolved
Hide resolved
stm32-modules/include/vacuum-module/vacuum-module/MPRLL0025PA00001A.hpp
Outdated
Show resolved
Hide resolved
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## edge #543 +/- ##
==========================================
+ Coverage 77.52% 77.84% +0.31%
==========================================
Files 83 86 +3
Lines 7743 7773 +30
==========================================
+ Hits 6003 6051 +48
+ Misses 1740 1722 -18 🚀 New features to boost your workflow:
|
sfoster1
left a comment
sfoster1
left a comment
There was a problem hiding this comment.
bunch of little things, but also it kind of seems like the end of conversion and reset pins are flipped?
| constexpr uint8_t ONE_SHOT_PRESSURE_READ = 0x01; | ||
| // pressure in hectoPascals is the 3 byte output value divided by the | ||
| // sensitivity | ||
| constexpr uint16_t SENSOR_SENSITIVITY = 4096.0f; |
There was a problem hiding this comment.
float constant but integral datatype
| constexpr uint8_t PRESSURE_FRAME_LEN = 10; | ||
|
|
||
| // Frame retry defaults | ||
| constexpr uint8_t DEFAULT_RETRIES = 3; |
There was a problem hiding this comment.
no need for this to be a uint8_t since it's used on line 51 for comparing with a regular int; the loop variable and this should be the same type, whatever that type happens to be
| } | ||
| } | ||
| if (!pressure_reading_ready) { | ||
| // raise an error here |
There was a problem hiding this comment.
even if we're not raising an error yet we should probably not parse whatever's randomly in the data buffer (last read?) even in dev since it might make the control loop run away. really recommend making this function return std::optional<double> or something
| // Add the header | ||
| WR_BUFF[0] = cmd; | ||
| // Copy data to the buffer starting from the header len. | ||
| for (uint8_t i = 1; i < len; i++) { |
There was a problem hiding this comment.
memcpy(&WR_BUFF[1], data, len)?
| auto I2C::i2c_master_write(uint16_t dev_addr, uint8_t *data, uint16_t size) | ||
| -> RxTxReturn { | ||
| auto ret = hal_i2c_master_write(bus, dev_addr, data, size); | ||
| auto ret = hal_i2c_master_write(this->bus, dev_addr, data, size); |
| return len + 1; | ||
| } | ||
|
|
||
| static auto parse_pressure(const uint8_t* raw) -> double { |
There was a problem hiding this comment.
this can be a const function right
| return len + 1; | ||
| } | ||
|
|
||
| static auto parse_pressure(const uint8_t* raw) -> double { |
There was a problem hiding this comment.
this can be a const function right
| // Equation 2: Pressure Output Function | ||
| auto press_counts = | ||
| // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-pointer-arithmetic) | ||
| (double)((int32_t)raw[3] + (int32_t)raw[2] * (int32_t)256 + |
There was a problem hiding this comment.
this is doing the same 24 bit bitshifting as the other one and it's way more readable if it's (int32_t)raw[3] + ((int32_t)raw[2]) << 8) + ((int32_t)raw[1]) << 16) or the same thing with | or whatever.
| // Add the header | ||
| WR_BUFF[0] = cmd; | ||
| // Copy data to the buffer starting from the header len. | ||
| for (uint8_t i = 1; i < len; i++) { |
There was a problem hiding this comment.
memcpy(&WR_BUFF[1], data, len)?
| constexpr uint8_t PRESSURE_FRAME_LEN = 10; | ||
|
|
||
| // Frame retry defaults | ||
| constexpr uint8_t DEFAULT_RETRIES = 3; |
There was a problem hiding this comment.
should be the same data type as the loop var on line 60 (don't care which)
caila-marashaj
force-pushed
the
vm-pressure-sensor-policies
branch
from
1c29a55 to
f198002
Compare
Overview
With this code, once a FreeRTOSTask is connected to the sensor policies, you should be able to read from all sensors, as well as interact with the eoc and reset pins on the MPR pressure sensor.
Another note: the atmosphere pressure sensor (lps22df)'s pins aren't actually connected to the MCU as of this hardware revision. There's just a TODO noting that we should add that when it's connected.
Changelog
system_hardwareto its own filemain.hand reference those, rather than theHAL_GPIOdefs directly