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

… use only one array of pins instead of 2

…publish dio event message, inc. partition sz

…ls into new functions

…alogIOAdd msg

examples/wippersnapper_debug/wippersnapper_debug.ino

tyeth · 2025-02-20T00:47:15Z

src/components/i2c/model.cpp

+  _msg_i2c_device_event
+      .i2c_device_events[_msg_i2c_device_event.i2c_device_events_count]
+      .type = sensor_type;
+  float value = GetValueFromSensorsEvent(sensor_type, &event);


not sure all adafruit_sensor value types will always be representable by a float, made me wonder what does a boolean become in float_value representation? Is it just nearly 1 or 0?
I don't thing we have a binary input yet, like a button over i2c, but it's on the roadmap (polling i2c in each loop, as though something was an analog pin over i2c for an ADC, or a button / keypress / rotary encoder change from seesaw device etc)

It's a good point. The float is derived from the struct sensors_event_t in Adafruit_Sensor. However, I do not see any issue adding a bool type to that union (i.e: bool_value) as the union does not care about the type, only that one is selected, and we are "flagging" that it is a boolean by using a sensor type enum.

Do you think this is something we should consider or implement right now?

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

tyeth · 2025-02-21T01:22:05Z

src/components/i2c/hardware.cpp

+#if defined(PIN_I2C_POWER)
+  // turn on the I2C power by setting pin to opposite of 'rest state'
+  pinMode(PIN_I2C_POWER, INPUT);
+  delay(1);


Does this work reliably, should we increase the time to stablise to 5ms, and could that help for the Feather S2 BME280 situation (I took a peek at support and twiddling gpio7/i2cPower as a component didn't help)? There is also a PIN_I2C_POWER_INVERTED defined as pin7 for the revTft S2 feather (probably same situation) which appears unused in code (v1)

Does this work reliably, should we increase the time to stablise to 5ms,
The code here follows the variant.cpp for each board (https://github.com/espressif/arduino-esp32/blob/master/variants/adafruit_feather_esp32s2/variant.cpp#L37). I don't want to modify this to be out-of-sync with this file.

There is also a PIN_I2C_POWER_INVERTED defined as pin7 for the revTft S2 feather (probably same situation) which appears unused in code (v1)

I am confused. For the RevTFT S2, the variant only defines a TFT_I2C_POWER pin
https://github.com/espressif/arduino-esp32/blob/master/variants/adafruit_feather_esp32s2_reversetft/variant.cpp#L36:37

And we handle it here in the code:

Adafruit_Wippersnapper_Arduino/src/components/i2c/hardware.cpp

Line 41 in d0b42ad

#elif defined(TFT_I2C_POWER)

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.

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 5s ⏱️
1 suites 0 💤
1 files 0 ❌

Results for commit 5ac35f3.

♻️ This comment has been updated with latest results.

brentru added 30 commits September 10, 2024 11:11

🚧 WIP, digitalio - Fix linker error, refactor get pin to get pin idx,…

1007802

… use only one array of pins instead of 2

🚧 WIP, digitalio - blink demo

1a6fc3d

🚧 WIP, digitalio: Add input, input pullup to ConfigurePin

c29f210

🚧 WIP, digitalio - Refactor ConfigurePin, AddDigitalPin

1363329

refactor adddigitalpin() call

60fb5f6

🚧 WIP, digitalio: Add Update() service for reading pins

3ac8472

🚧 WIP, digitalio - Add encoder for digital io event to model, encode/…

10a19bf

…publish dio event message, inc. partition sz

🚧 WIP, digitalio - Refactor out the Update() conditional function cal…

434107c

…ls into new functions

🚧 WIP, digtialio: adhere to period better, fix bug

5288af0

🚧 WIP, digitalio - publishing to broker missing value

a3386d3

looking at flash failure

3093b50

continue fix flash failure

1b2c10b

🚑️ Fix Filesystem bug in V2

d872814

🎨 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

brentru added 14 commits February 18, 2025 11:34

fix rename

5f78c96

tree clang

1d12fa7

checkout 2x?

1d2d97c

do not clang inside protos/ dir, new dir replaces wippersnapper/ dir

670a6dc

run clang first

77bbf1a

clang, fix offline test runner to install

af886db

bring in test runner, attempt to run!

2770d8a

clang again

f206963

clangx2

61de287

target esp32dev install only

33f5955

tests use the runner

8e7385c

Pass tests

cec28b1

consolidate tests and build script

357a73c

bump download-artifact version to 4

d0b42ad

tyeth reviewed Feb 19, 2025

View reviewed changes

examples/wippersnapper_debug/wippersnapper_debug.ino Outdated Show resolved Hide resolved

tyeth reviewed Feb 20, 2025

View reviewed changes

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

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 3 commits February 24, 2025 14:19

Modify demos

8aa233a

attribute scott perkins correctly

a2e722c

build while testing instead

5ac35f3

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 20, 2025

brentru Feb 20, 2025

brentru Feb 20, 2025

tyeth Feb 21, 2025 •

edited

Loading

brentru Feb 21, 2025 •

edited

Loading

tyeth Feb 21, 2025

brentru Feb 21, 2025

tyeth Feb 21, 2025 •

edited

Loading

brentru Feb 21, 2025

brentru commented Feb 21, 2025

brentru commented Feb 24, 2025

github-actions bot commented Feb 24, 2025 •

edited

Loading

API V2 - I2C Review #693

Are you sure you want to change the base?

API V2 - I2C Review #693

Conversation

brentru commented Feb 13, 2025 • edited Loading

Pull Request Scope

Code Review Scope

(Optional) Hardware Testing

tyeth Feb 20, 2025

Choose a reason for hiding this comment

brentru Feb 20, 2025

Choose a reason for hiding this comment

brentru Feb 20, 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 • edited Loading

Choose a reason for hiding this comment

tyeth Feb 21, 2025

Choose a reason for hiding this comment

brentru Feb 21, 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

brentru commented Feb 21, 2025

brentru commented Feb 24, 2025

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

Test Results

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