Repository navigation
Conversation
|
Hi @kouhei1970 , could you help to review this MR :) |
kouhei1970
left a comment
There was a problem hiding this comment.
Thank you for this careful work — supporting Arduino-ESP32 3.x while keeping the 2.x path intact is very welcome, and isolating the I2C access through esp32-hal-i2c (instead of mixing the new and legacy IDF drivers) looks like the right approach. I reviewed the code and built both environments locally (PlatformIO 6.1.19): esp32-s3-devkitc-1 (Arduino-ESP32 2.0.17) and esp32-s3-devkitc-1-arduino3311 both build successfully. I have not flashed them on hardware myself yet. A few points before merging:
Should fix
-
ESP-NOW send callback guard (
src/rc.cpp:57,src/rc.cpp:166)
The guard isESP_IDF_VERSION >= ESP_IDF_VERSION_VAL(5, 0, 0), butesp_now_send_info_twas introduced in ESP-IDF 5.5. In IDF 5.1–5.4 (Arduino-ESP32 3.0–3.2)esp_now_send_cb_tstill takesconst uint8_t *mac_addr, so the build fails there. I confirmed this with Arduino-ESP32 3.1.3 (pioarduino53.03.13, IDF 5.3):src/rc.cpp:58:28: error: 'esp_now_send_info_t' does not name a type; did you mean 'esp_now_recv_info_t'?Suggest
ESP_IDF_VERSION_VAL(5, 5, 0)for the send callback (the receive callback change toesp_now_recv_info_tin IDF 5.0 is fine as is). -
I2C scan in
tof_init()(src/tof.cpp:80-82)
scan_i2c()and the log line are not inside a version guard, so they also run on the 2.x path. This looks like debug code; could you remove it or put it behind a debug flag, so that the 2.x behavior stays unchanged as described? -
CI workflows
- Arduino Lint: this repository is a PlatformIO project, not an Arduino library/sketch, so
arduino-lintfails with "No projects found". It never ran before because it was only triggered onmaster. Addingmainto the triggers turns it into a check that cannot pass. I would suggest reverting the trigger change in this PR (whether to delete or reconfigure the workflow is up to the maintainers). - clang-format: thanks for fixing the duplicated
excludekey — the workflow was invalid YAML onmainand never actually ran. Now it runs and reports many pre-existing files, which is out of scope here, but could you at least format the new filesrc/arduino_esp32_i2c_compat.c?
- Arduino Lint: this repository is a PlatformIO project, not an Arduino library/sketch, so
Questions
-
I2C timeout inside the 400 Hz loop (
src/arduino_esp32_i2c_compat.c:8)
The ToF range is read fromsensor_read(), which is called fromloop_400Hz(). With a 50 ms timeout, an I2C fault in flight would block the control loop for up to 20 control periods (2.5 ms each), compared with about 1 tick in the original code. I agree it is only an upper bound, but have you considered a shorter timeout for runtime reads (keeping the longer one only for initialization)? -
Hardware testing of the 2.x environment
The PR also changes the 2.x path (the scan above, anddigitalWrite(buzzerPin, 0)insrc/buzzer.cpp:39— the latter fixes an obvious bug, since the original wrote GPIO5 = front-left motor pin). Were the hardware tests in the checklist done with both environments, or only with 3.3.11?
Nits
- The three new files are added as
100755(the description says permissions were restored to100644). - Please write the new comments in
src/rc.cppin English (the rest of the code uses Japanese/English). platformio.inihas no newline at end of file.
P.S. A note mainly for the M5Stack team: honestly, I have been thinking that the firmware shipped on StampFly (this repository) should eventually be replaced with the vehicle firmware from stampfly_ecosystem. That is a separate discussion from this PR, though. Until then, this PR is still valuable for everyone who uses the current firmware with newer Arduino-ESP32 versions.
- Switch the esp_now_send_info_t callback guard from IDF 5.0.0 to 5.5.0 - Fix build on Arduino-ESP32 3.0–3.2 (IDF 5.1–5.4), which still use const uint8_t *mac_addr - Restore 100644 mode on the three new compat source/header files
- Drop the scan_i2c() call and log after the front ToF address change - Drop the sensor.hpp include that was only needed for it - Keep 2.x ToF init behavior unchanged
- Restore Arduino-Lint-Check.yml to the upstream version - Avoid an always-failing check on main, since this is a PlatformIO project
- Apply the repository .clang-format with clang-format 13 (same as CI) - Whitespace-only change
- Replace the two Chinese comments added around the ESP-NOW receive handler
- Keep a 50 ms I2C timeout during sensor initialization - Switch to 5 ms at the end of sensor_init() before the 400 Hz loop starts - Add stampfly_i2c_set_timeout_ms() to the I2C compat layer - Leave the Arduino 2.x path unchanged
- Add a "Recommended firmware" section linking to M5Fly-kanazawa/stampfly_ecosystem
|
@kouhei1970 Thank you for the detailed review and for building both environments locally. I also apologize for the careless mistakes in the previous revision. All review points have been addressed as follows. Should fix
Questions
Nits
Regarding the P.S., I have also added a note to the README recommending the vehicle firmware in stampfly_ecosystem. Update on the front ToF issuePlease disregard the front ToF issue I described earlier, if you have already read it. It turned out to be a power supply issue: during that test, the board was powered only over USB, without the battery connected. With the battery connected, the original code works fine: the front ToF stays at 0x2A, there are no I2C errors, and both the bottom and front ranges are valid. So no code change is needed for this, and this PR does not include one. |
Summary
This PR adds support for Arduino-ESP32 3.3.11 while preserving the existing Arduino-ESP32 2.x build environment and runtime behavior.
The update addresses API and driver changes introduced in Arduino-ESP32 3.x, including I2C, hardware timers, LEDC/PWM, USB CDC, ESP-NOW callbacks, and Wi-Fi station MAC retrieval. It also adds CI coverage for both framework generations.
Motivation
When running the original firmware on Arduino-ESP32 3.x, several compatibility problems were observed:
WiFi.macAddress()could return an all-zero address during startup, causing the controller to save an invalid ESP-NOW peer address.Changes
Arduino-ESP32 compatibility
ToF and I2C
0x29to0x2A.ESP-NOW
esp_mac.hsupport for Arduino-ESP32 3.x.esp_read_mac(..., ESP_MAC_WIFI_STA).WiFi.macAddress()path for Arduino-ESP32 2.x.CI and repository cleanup
mainormaster.excludekey in the clang-format workflow.100755to100644.Backward Compatibility
The existing Arduino-ESP32 2.x environment and implementation paths remain available. Version-specific behavior is isolated through conditional compilation.
This PR does not intentionally change:
Validation
git diff --checkpasses.Notes
The configured I2C timeout is a maximum wait time, not an unconditional delay. Successful transfers return as soon as the underlying transaction completes.