Scd40 driver libhal 2.0 #108
Conversation
This reverts commit 75b7bd0.
…/urc-control-system-2023 into scd40-driver-libhal-2.0
There was a problem hiding this comment.
This isn't a full review, but some things I spotted when I skimmed through this PR. Great work @MichaelYKersey ! Keep it up :)
|
|
||
| hal::status scd40_nm::set_settings( struct settings setting) { | ||
| if(setting.set_temp != 0){ | ||
| int temp = int(((setting.set_temp*(1<<16)) / 175)) ; |
There was a problem hiding this comment.
| int temp = int(((setting.set_temp*(1<<16)) / 175)) ; | |
| int temp = int(((setting.set_temp*(1<<16)) / 175)) ; |
Give these magic numbers names. 175 isn't some power of two so it must mean something. Fine the term in the datasheet and make a constexpr variables for it. Something like:
// Found on page X in datasheet name "<url to datasheet>"
constexpr std::uint32_t temperature_scalar = 175; |
|
||
|
|
||
| //TODO: revise output type | ||
| enum addresses {// deal with hal::byte later |
There was a problem hiding this comment.
| enum addresses {// deal with hal::byte later | |
| enum class addresses { // deal with hal::byte later |
Great work using an enum for all of the addresses. Makes it easier to understand whats what. But its favorable to use an enum class as they get promoted to a type. You can static_cast<> them back to whatever integral repersation you need provided that representation does not truncate the enum value. You can also use hal::value() in <libhal-util/enum.hpp>
There was a problem hiding this comment.
I talked to Corey and Gene to understand what you were saying. While on the topic they recommended using constants (likely in a separate file) for this use case. What are your thoughts on switching the values in the enum to static constants?
There was a problem hiding this comment.
static constants are great! Sometimes the constructs of an enum work fine too just depends on what you are doing.
| double set_temp = 4; | ||
| double set_alt = 0; | ||
| double set_pressure = -1; |
There was a problem hiding this comment.
What do these magic numbers mean? Maybe make some constants or add some comments?
| static hal::result<scd40> create(hal::i2c& p_i2c,hal::steady_clock& p_clock); | ||
| hal::status start(); | ||
| hal::result<scd40_read_data> read(); | ||
| hal::status stop(); | ||
| hal::result<scd40_settings> get_settings(); | ||
| hal::status set_settings( struct settings setting); |
There was a problem hiding this comment.
These APIs should be documented in order to understand how they should be used.
Scd40 driver: functional readings, config settings are untested