Skip to content

scd4x: adhere to the datasheet better - #895

Open
sparques wants to merge 4 commits into
tinygo-org:devfrom
sparques:dev
Open

sparques wants to merge 4 commits into
tinygo-org:devfrom
sparques:dev

Conversation

@sparques

Copy link
Copy Markdown

I was having issues using the scd4x driver; compared what it was doing to what the datasheet says should be done and fixed all the places where the two differed.

  • Checks DataReady() before reading.
  • Returns ErrDataNotReady without restarting the sensor.
  • Validates CRCs on all returned words.
  • Waits the required 30 ms after reinit.
  • Uses the correct 65535 conversion denominator.
  • Waits 30 ms for power-up before starting measurements.
  • Removes the unnecessary startup Configure() call.
  • Adds CRC and conversion tests.

- Checks DataReady() before reading.
- Returns ErrDataNotReady without restarting the sensor.
- Validates CRCs on all returned words.
- Waits the required 30 ms after reinit.
- Uses the correct 65535 conversion denominator.
- Waits 30 ms for power-up before starting measurements.
- Removes the unnecessary startup Configure() call.
- Adds CRC and conversion tests.
@conejoninja

Copy link
Copy Markdown
Member

I prefer the int32 measure, I think the precision lose in under the margin error of the sensor, and is less ram/cycles hungry. What does anyone else think?

@deadprogram

Copy link
Copy Markdown
Member

Thanks for going through the datasheet on this. The CRC checks, the 30 ms wait after reinit, and checking DataReady() are all good changes.

A few things before we merge:

  1. The import comment changed from // import "tinygo.org/x/drivers/scd4x" to //import .... Please revert that, it's unrelated and the rest of the repo uses the // import form.

  2. I agree with @conejoninja on keeping the int32 conversions. Dividing by 65536 instead of 65535 is off by less than 0.003 °C and 0.0015 %RH, which is far below the sensor accuracy of about ±0.8 °C and ±6 %RH. The int64 math is slower and larger on Cortex-M0 for no real gain.

  3. Please keep the existing test using quicktest and tester.NewI2CBus since that is what the rest of the repo uses. The new CRC and conversion tests are welcome, they can use the same setup.

  4. The description mentions waiting 30 ms for power-up before starting measurements and removing the startup Configure() call, but I don't see those in the diff. Were some changes left out?

One question. Update() now returns ErrDataNotReady when no new sample is available. Anyone polling faster than every 5 seconds will get an error on most calls. Would it be better to return nil and keep the cached values? That seems closer to what the doc comment on Update() says.

@sparques

Copy link
Copy Markdown
Author

I addressed the review comments. The 30ms for power up I just forgot to commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants