dbrgn / shtcx-rs

Platform agnostic Rust driver for the Sensirion SHTCx temperature/humidity sensors.
Apache License 2.0
13 stars 4 forks source link

Add type parameters for SHTC1 / SHTC3 #10

Closed dbrgn closed 4 years ago

dbrgn commented 4 years ago

Fixes #7.

codecov[bot] commented 4 years ago

Codecov Report

Merging #10 into master will decrease coverage by 2.32%. The diff coverage is 92.75%.

@@            Coverage Diff            @@
##           master     #10      +/-   ##
=========================================
- Coverage   89.62%   87.3%   -2.32%     
=========================================
  Files           1       3       +2     
  Lines         106     126      +20     
=========================================
+ Hits           95     110      +15     
- Misses         11      16       +5
Impacted Files Coverage Δ
src/types.rs 100% <100%> (ø)
src/crc.rs 100% <100%> (ø)
src/lib.rs 84.16% <88.64%> (-5.46%) :arrow_down:
dbrgn commented 4 years ago

I'm a tiny bit worried about code size explosion due to monomporphisation (does different code get generated for zero sized types?).

Probably, the alternative would be compile flags, but you didn't like those :stuck_out_tongue:

But it should seldomly matter since one typically uses just one sensor.

Yep. And the idea was to provide a generic variant with some tolerance in timing that should work for all sensors (in case you don't know in advance which one you'll have).

dbrgn commented 4 years ago

@rnestler after a lot of different trait based approaches I switched to a macro based approach. For example, the sleep and wakeup commands are implemented for the ShtC3 and the ShtGeneric using a macro. To make the docs easier to read, I moved them into a separate trait called LowPower.

Now the next question would be how to design the API for measurements. Right now there are three methods available on all sensors:

pub fn measure(&mut self, mode: PowerMode) -> Result<Measurement, Error<E>>;
pub fn measure_temperature(&mut self, mode: PowerMode) -> Result<Temperature, Error<E>>;
pub fn measure_humidity(&mut self, mode: PowerMode) -> Result<Humidity, Error<E>>;

The problem is that they all require a PowerMode which doesn't make much sense on the SHTC1.


One idea would be to only use normal mode in the normal methods (available on all sensors), and to move the low power measurements into the LowPower trait:

pub fn measure(&mut self) -> Result<Measurement, Error<E>>;
pub fn measure_temperature(&mut self) -> Result<Temperature, Error<E>>;
pub fn measure_humidity(&mut self) -> Result<Humidity, Error<E>>;

trait LowPower {
    pub fn measure_lp(&mut self) -> Result<Measurement, Error<E>>;
    pub fn measure_temperature_lp(&mut self) -> Result<Temperature, Error<E>>;
    pub fn measure_humidity_lp(&mut self) -> Result<Humidity, Error<E>>;
}

This would be OK, but we'd lose the ability to parametrize the PowerMode of the measurement.


Another approach would be to generate different measurement commands for the different sensors:

impl ShtCx<ShtC1> {
    pub fn measure(&mut self) -> Result<Measurement, Error<E>>;
    pub fn measure_temperature(&mut self) -> Result<Temperature, Error<E>>;
    pub fn measure_humidity(&mut self) -> Result<Humidity, Error<E>>;
}

impl ShtCx<ShtC3> {
    pub fn measure(&mut self, mode: PowerMode) -> Result<Measurement, Error<E>>;
    pub fn measure_temperature(&mut self, mode: PowerMode) -> Result<Temperature, Error<E>>;
    pub fn measure_humidity(&mut self, mode: PowerMode) -> Result<Humidity, Error<E>>;
}

impl ShtCx<ShtGeneric> {
    pub fn measure(&mut self, mode: PowerMode) -> Result<Measurement, Error<E>>;
    pub fn measure_temperature(&mut self, mode: PowerMode) -> Result<Temperature, Error<E>>;
    pub fn measure_humidity(&mut self, mode: PowerMode) -> Result<Humidity, Error<E>>;
}

This would be more ergonomic (the PowerMode is simply not part of the API for the SHTC1), but the downside is that the docs get quite ugly, because every measure method is listed n times. (Note that the impls could be generated with a macro, so that would be fine.)


As a third option, we could choose the first option with parametrized methods:

pub fn measure(&mut self) -> Result<Measurement, Error<E>>;
pub fn measure_temperature(&mut self) -> Result<Temperature, Error<E>>;
pub fn measure_humidity(&mut self) -> Result<Humidity, Error<E>>;

trait LowPower {
    pub fn measure_with_mode(&mut self, mode: PowerMode) -> Result<Measurement, Error<E>>;
    pub fn measure_temperature_with_mode(&mut self, mode: PowerMode) -> Result<Temperature, Error<E>>;
    pub fn measure_humidity_with_mode(&mut self, mode: PowerMode) -> Result<Humidity, Error<E>>;
}

The method names look ugly though. (Unfortunately we cannot call them measure as well without bad consequences for ergonomics, since there's no overloading in Rust.)


Any ideas?

rnestler commented 4 years ago

The problem is that they all require a PowerMode which doesn't make much sense on the SHTC1.

Why doesn't it make sense on the SHTC1? You are aware that it supports the same measure modes as the SHTC3, right? See the datasheet:

These values can be reduced by using the low power measurement mode, see separate application note.

And the mentioned application note

dbrgn commented 4 years ago

WTF, that datasheet is very confusing.

SHTC3

2020-02-05-132539_425x212_scrot

2020-02-05-132615_630x191_scrot

2020-02-05-132636_874x60_scrot

SHTC1

2020-02-05-132714_409x157_scrot

2020-02-05-132731_871x139_scrot

2020-02-05-132803_881x57_scrot

The only reference to low power mode is in a small footnote.

If that's really the case, then the whole refactoring doesn't make sense :confused: So in the end, the only difference in the protocol between SHTC1 and SHTC3 are missing sleep/wakeup commands and different max measurement timings?

rnestler commented 4 years ago

So in the end, the only difference in the protocol between SHTC1 and SHTC3 are missing sleep/wakeup commands and different max measurement timings?

Yes, as far as I'm aware.