API V2 - I2C Review #693

brentru · 2025-02-13T18:05:31Z

Resolves #640

Pull Request Scope

Added new classes for interfacing with I2C drivers within src/i2c and refactored all existing drivers to fit the new modified driver base class (drvBase) within src/i2c/drivers
- Major Changes:
  - This is a breaking change and complete rewrite of WipperSnapper's I2C component
  - I2C components now use the latest version of the i2c.proto message API
  - I2C drivers may optionally be created on an alternative I2C port
  - I2C drivers may optionally be created on an I2C Multiplexer
  - I2C drivers now share one-period-per-device, rather than multiple-periods-per-device
  - I2C components may be (optionally) initialized from a config.json file on an SD card
  - I2C components may now (optionally) log data to an SD card
  - I2C components log data into a unified SensorEvent message, from sensor.proto. This message can then be - published to MQTT, logged to sd, and/or published to the serial
Added handler for parsing I2C messages within a config.json file into a protobuf message within class ws_sd

This pull request only covers "offline mode" functionality, interfaces for publishing back to the MQTT broker have not yet been fully implemented.

Code Review Scope

This pull request adds the following new files within src/i2c/ and replaces all existing files:

I2C Model - Provides interfaces for interacting with messages within the i2c.proto API

model.cpp
model.h

I2C Hardware - Manages low-level i2c bus functionality

hardware.cpp
hardware.h

I2C Controller - Manages high-level functionality for handling I2C messages

controller.cpp
controller.h

There are also refactored drivers the within src/i2c/drivers/ folder. I do not expect all of the drivers within this folder to be reviewed as a subset was tested for functionality. However, drvBase.h should be reviewed.

drvBmp280.h
drvAdt7410.h
drvAhtx0.h
drvBase.h
drvBh1750.h
drvBme280.h
drvBme680.h
drvBmp3xx.h
drvDps310.h
drvDs2484.h
drvEns160.h
drvHts221.h
drvHtu21d.h
drvIna219.h
drvLc709203f.h
drvLps3xhw.h
drvLps22hb.h
drvLps25hb.h
drvLtr329_Ltr303.h
drvLtr390.h
drvMax1704x.h
drvMcp3421.h
drvMCP9808.h
drvMpl115a2.h
drvMprls.h
drvMs8607.h
drvNau7802.h
drvPct2075.h
drvPm25.h
drvScd4x.h
drvScd30.h
drvSen5x.h
drvSgp40.h
drvSht3x.h
drvSht4x.h
drvShtc3.h
drvSi7021.h
drvStemmaSoil.h
drvTmp117.h
drvTsl2591.h
drvVeml7700.h
drvVl53l0x.h
drvVl53l1x.h
drvVl53l4cd.h
drvVl53l4cx.h
drvVl6180x.h
drvVncl4020.h
drvVncl4040.h

Modifies functions within the ws_sdcard class:

parseConfigFile()

Adds new functions within the ws_sdcard class:

ParseI2cDeviceAddReplace()
LogI2cDeviceEvent()

(Optional) Hardware Testing

If you'd like to test this on hardware, I suggest building the firmware using PlatformIO. I have tested on: QT Py ESP32-S2 + RTC + SD BFF, Raspberry Pi Pico 2

Notes:

The config.json should reside on the WIPPER drive, not the MicroSD card.
Ensure that the RTC has a battery and the the MicroSD card is inserted prior to running WipperSnapper
For each of the configuration files, please note that you will need to modify rtc and sd_cs_pin variables to match the name of the RTC you are using and the pin the MicroSD Card breakout is utilizing

        "rtc": "DS3231",
        "sd_cs_pin": 17,

Example config.json - SHTC3 on default I2C Port
shtc3.json

Example config.json - MCP9808 connected to PCA9546 MUX on an alternate I2C Port
muxpca-mcp.json

Example config.json - BME6xx
muxpca-mcp.json

Example config.json - AHT20
aht20.json

Example config.json - 2x Sensors on one bus, BME280 and TSL2591
config-bme280-tsl.json

…alogIOAdd msg

…ation

#635

…low bug in func signature mismatch for model

src/components/i2c/drivers/drvBh1750.h

tyeth · 2025-02-21T01:11:19Z

src/components/i2c/hardware.cpp

+void I2cHardware::InitBus(bool is_default, const char *sda, const char *scl) {
+  uint8_t pin_sda, pin_scl;
+  if (!is_default && (sda == nullptr || scl == nullptr)) {
+    _bus_status = wippersnapper_i2c_I2cBusStatus_I2C_BUS_STATUS_ERROR_WIRING;


I think this might be the wrong message, if nullptr then we haven't set the alt bus sda/scl pins, i.e. corrupt protobuf msg from offline config?

I'm not sure what is better here.

What I2cBusStatus should we set instead? We could also add to I2cBusStatus:
https://github.com/adafruit/Wippersnapper_Protobuf/blob/api-v2/proto/wippersnapper/i2c.proto#L10

I guess back to the default of 0 (wippersnapper_i2c_I2cBusStatus_I2C_BUS_STATUS_UNSPECIFIED) might be better for now. Wasn't sure what to add rather than utilise existing.

src/components/i2c/hardware.cpp

tyeth · 2025-02-21T01:36:28Z

src/components/i2c/hardware.cpp

+    _bus_status = wippersnapper_i2c_I2cBusStatus_I2C_BUS_STATUS_ERROR_HANG;
+    return;
+  }
+  _bus->setClock(50000);


I know this is from V1, I wonder if we need this optionally per sensor or something, but I was sure there were a couple with very strict requirements (couple needed 100k in theory and maybe another one had issues except around 20k). Ignore me for now, but if it rings a bell or comes up again then shout.
In my head we were running at 100kHz for everything, but I must have been thinking of circuitpython or something

The specification was to run at 500kHz for everything on the bus. API v1 did contain a message that sets the I2C bus' frequency (https://github.com/adafruit/Wippersnapper_Protobuf/blob/master/proto/wippersnapper/i2c/v1/i2c.proto#L45) but since we are always sending 500kHz in that message, it got dropped in API v2.

Got you! I'm assuming you're aware that you're talking about 5 hundred, but mean to say 50kHz like the code is fifty kHz, but yep.

brentru · 2025-02-21T16:20:16Z

@tyeth Noting that we discussed the following over Slack Huddle:
a) A potentially unused constructor in drvBase (the one w/o mux ch. defined)
b) drvBase inherited classes potentially should not have anything in their constructors

brentru · 2025-02-24T18:48:36Z

The two issues mentioned above have been resolved.

github-actions · 2025-02-24T19:05:32Z

Test Results

6 tests 6 ✅ 1m 6s ⏱️
1 suites 0 💤
1 files 0 ❌

Results for commit ad0e794.

♻️ This comment has been updated with latest results.

tyeth · 2025-02-27T21:47:33Z

Sticking this in here so it isn't missed, the rtc not being initialised with a time via some alternative means causes a failure. We should instead initialise it at zero or ideally use a soft RTC instead and the user can work it out from the timestamps or seeing serial warning. We should not set the RTC to time at compile time.

src/components/i2c/drivers/drvBase.h

src/components/i2c/hardware.cpp

tyeth · 2025-02-26T19:29:53Z

src/components/i2c/hardware.cpp

+void I2cHardware::InitBus(bool is_default, const char *sda, const char *scl) {
+  uint8_t pin_sda, pin_scl;
+  if (!is_default && (sda == nullptr || scl == nullptr)) {
+    _bus_status = wippersnapper_i2c_I2cBusStatus_I2C_BUS_STATUS_ERROR_WIRING;


I guess back to the default of 0 (wippersnapper_i2c_I2cBusStatus_I2C_BUS_STATUS_UNSPECIFIED) might be better for now. Wasn't sure what to add rather than utilise existing.

tyeth · 2025-02-26T19:33:38Z

src/components/i2c/hardware.cpp

+    _bus_status = wippersnapper_i2c_I2cBusStatus_I2C_BUS_STATUS_ERROR_HANG;
+    return;
+  }
+  _bus->setClock(50000);


Got you! I'm assuming you're aware that you're talking about 5 hundred, but mean to say 50kHz like the code is fifty kHz, but yep.

src/components/i2c/hardware.cpp

src/components/i2c/controller.cpp

brentru added 30 commits September 25, 2024 11:36

🎨 Improve structure of the FS code

50609f7

🚧 WIP, digitalio - Sends boolean value from timer-based input

fcc6914

🚧 WIP, digitalio - Refactoring in update()

0675abc

🚧 WIP, digitalio - Add doxygen comments

70406f3

🚧 WIP, digitalio - refactoring

624ba3d

🚧 WIP, digitalio - Address TODO for statusled

e513533

🎨 Add doxygen across new v2 classes

ce7d5ed

🎨 Doxygen sensor model, ws_manager classes

47dcb93

🎨 Doxygen pass complete

fbaadb9

🚧 WIP, digitalio - Implement remove pin API

4a2ccac

🚧 WIP, digitalio - Attach dio remove to b2d cb

e42e963

🚧 WIP, digitalio - refactor naming to match python

3cc50de

rm unused model func

c20e45b

🚧 WIP, analogio - Create controller, hardware, model and hooks for An…

49e110d

…alogIOAdd msg

⬆️ Upgrade protobuf to match ee1df09fb542fd9ba27271cfa5aebc75bd9842a3

6b93efc

⬆️ Upgrade protos, 7dd0fe6

b9f7c12

🚧 WIP, analogio - Fix compiler errors and build pass

4ca038c

🚧 WIP, analogio - Add deinit, scale reads, update

fcd87d1

🚧 WIP, analogio - Read voltage, precalculate scale factor for optimiz…

eb6bf12

…ation

🎨 WIP, analogio - Encode, publish voltage event

e5e2724

🚧 WIP, analogio - Add encoder for a raw analogio event

fac050e

🚧 WIP, analogio - refactor encode/publish in controller, refactor model

e5d3199

🚧 WIP, analogio - publish out

6a82e77

🎨 WIP, analogio - Doxygen analogio classes

067c220

🐛 WIP, analogio - bring in

549874a

#635

🐛 WIP, analogio - Bug fixes during testing

e30d4e4

🐛 WIP, analogio - Refactor hardware.cpp for raw adc reads, fix underf…

7f2d550

…low bug in func signature mismatch for model

add partitions, use larger parition size

706652f

🚧 WIP, dsx - Sketch out ds18x20 classes

a9288cc

🚧 WIP, dsx - Plumbing for ds18x in App, switch to OneWireNG interface

e342564

tyeth reviewed Feb 21, 2025

View reviewed changes

src/components/i2c/drivers/drvBh1750.h Outdated Show resolved Hide resolved

tyeth reviewed Feb 21, 2025

View reviewed changes

src/components/i2c/hardware.cpp Show resolved Hide resolved

tyeth reviewed Feb 21, 2025

View reviewed changes

brentru added 2 commits February 24, 2025 13:23

Work without a RTC attached

6485e4d

Remove unused ctor in drvBase, remove code in ctor body for drvers

9c3d525

brentru added 2 commits February 24, 2025 13:57

Add workflow for this branch

0d96f93

remove .cpp residual

afffa81

brentru added 14 commits February 24, 2025 14:19

Modify demos

8aa233a

attribute scott perkins correctly

a2e722c

build while testing instead

5ac35f3

Fix I2C Pinout on RP2040 Variants

affdf95

Add Adafruit RP2040 Adalogger to PIO and codebase

13b1496

Add Adafruit RP2040 Adalogger to workflow too

f0abfa5

putback demo code

835b601

Allow for SPI1 port to be used on Feather RP2040 Adalogger

8d261ec

skip/gen files

d88b17f

rename skip/gen

1518461

Add Metro RP2350

4093c96

Add Metro RP2350 builds

99d8cf5

clang

d3cdc61

attempt to fix rp2040 adalogger

9f6a088

tyeth approved these changes Mar 3, 2025

View reviewed changes

brentru added 2 commits March 3, 2025 10:50

Address Tyeth feedback

02c09e6

Mirror library.properties on main

ad0e794

brentru closed this Mar 3, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API V2 - I2C Review #693

API V2 - I2C Review #693

brentru commented Feb 13, 2025 •

edited

Loading

tyeth Feb 21, 2025 •

edited

Loading

brentru Feb 21, 2025 •

edited

Loading

tyeth Feb 26, 2025

tyeth Feb 21, 2025 •

edited

Loading

brentru Feb 21, 2025

tyeth Feb 26, 2025

brentru commented Feb 21, 2025

brentru commented Feb 24, 2025

github-actions bot commented Feb 24, 2025 •

edited

Loading

tyeth commented Feb 27, 2025

tyeth Feb 26, 2025

tyeth Feb 26, 2025

API V2 - I2C Review #693

API V2 - I2C Review #693

Conversation

brentru commented Feb 13, 2025 • edited Loading

Pull Request Scope

Code Review Scope

(Optional) Hardware Testing

tyeth Feb 21, 2025 • edited Loading

Choose a reason for hiding this comment

brentru Feb 21, 2025 • edited Loading

Choose a reason for hiding this comment

tyeth Feb 26, 2025

Choose a reason for hiding this comment

tyeth Feb 21, 2025 • edited Loading

Choose a reason for hiding this comment

brentru Feb 21, 2025

Choose a reason for hiding this comment

tyeth Feb 26, 2025

Choose a reason for hiding this comment

brentru commented Feb 21, 2025

brentru commented Feb 24, 2025

github-actions bot commented Feb 24, 2025 • edited Loading

Test Results

tyeth commented Feb 27, 2025

tyeth Feb 26, 2025

Choose a reason for hiding this comment

tyeth Feb 26, 2025

Choose a reason for hiding this comment

brentru commented Feb 13, 2025 •

edited

Loading

tyeth Feb 21, 2025 •

edited

Loading

brentru Feb 21, 2025 •

edited

Loading

tyeth Feb 21, 2025 •

edited

Loading

github-actions bot commented Feb 24, 2025 •

edited

Loading