Skip to content

Cleanup I2Cscan library #459

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Cleanup I2Cscan library #459

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3d0fd52
Cleanup scanI2C library
jlitewski Jun 4, 2025
50a6dd1
Fixed formatting issues
jlitewski Jun 5, 2025
be3c6fc
First round of changes based on feedback
jlitewski Jun 5, 2025
d21cdb7
Fixed a fat fingered mistake
jlitewski Jun 5, 2025
b84808c
Actually fix the formatting issues
jlitewski Jun 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
189 changes: 108 additions & 81 deletions lib/i2cscan/i2cscan.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,137 +1,164 @@
#include "i2cscan.h"

#include <array>
#include <cstddef>
#include <cstdint>
#include <string>

#include "../../src/globals.h"

namespace {
#ifdef ESP8266
uint8_t portArray[] = {16, 5, 4, 2, 14, 12, 13};
uint8_t portExclude[] = {LED_PIN};
String portMap[] = {"D0", "D1", "D2", "D4", "D5", "D6", "D7"};
std::array<uint8_t, 7> portArray = {16, 5, 4, 2, 14, 12, 13};
std::array<std::string, 7> portMap = {"D0", "D1", "D2", "D4", "D5", "D6", "D7"};
std::array<uint8_t, 1> portExclude = {LED_PIN};
#elif defined(ESP32C3)
uint8_t portArray[] = {2, 3, 4, 5, 6, 7, 8, 9, 10};
uint8_t portExclude[] = {18, 19, 20, 21, LED_PIN};
String portMap[] = {"2", "3", "4", "5", "6", "7", "8", "9", "10"};
std::array<uint8_t, 9> portArray = {2, 3, 4, 5, 6, 7, 8, 9, 10};
std::array<std::string, 9> portMap = {"2", "3", "4", "5", "6", "7", "8", "9", "10"};
std::array<uint8_t, 5> portExclude = {18, 19, 20, 21, LED_PIN};
#elif defined(ESP32C6)
uint8_t portArray[] = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 14, 15, 18, 19, 20, 21, 22, 23};
String portMap[] = {"0", "1", "2", "3", "4", "5", "6", "7", "8", "9", "10", "11", "14", "15", "18", "19", "20", "21", "22", "23"};
uint8_t portExclude[] = {12, 13, 16, 17, LED_PIN};
std::array<uint8_t, 20> portArray = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 14, 15, 18, 19, 20, 21, 22, 23};
std::array<std::string, 20> portMap = {"0", "1", "2", "3", "4", "5", "6", "7", "8", "9", "10", "11", "14", "15", "18", "19", "20", "21", "22", "23"};
std::array<uint8_t, 4> portExclude = {12, 13, 16, 17, LED_PIN};
#elif defined(ESP32)
uint8_t portArray[] = {4, 13, 14, 15, 16, 17, 18, 19, 21, 22, 23, 25, 26, 27, 32, 33};
String portMap[] = {"4", "13", "14", "15", "16", "17", "18", "19", "21", "22", "23", "25", "26", "27", "32", "33"};
uint8_t portExclude[] = {LED_PIN};
std::array<uint8_t, 16> portArray = {4, 13, 14, 15, 16, 17, 18, 19, 21, 22, 23, 25, 26, 27, 32, 33};
std::array<std::string, 16> portMap = {"4", "13", "14", "15", "16", "17", "18", "19", "21", "22", "23", "25", "26", "27", "32", "33"};
std::array<uint8_t, 1> portExclude = {LED_PIN};
#endif
}

namespace I2CSCAN
{
enum class ScanState {
namespace I2CSCAN {
enum class ScanState : uint8_t {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't pushed through the network, why give it a defined underlying type?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than not needing the full 4 bytes of space to do what 1 byte could?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are on a 32-bit architecture. Even if those 3 bytes we lose would be a huge deal, it's very very likely that turning it into a uint8_t isn't actually saving anything.

IDLE,
SCANNING,
DONE
};

ScanState scanState = ScanState::IDLE;
uint8_t currentSDA = 0;
uint8_t currentSCL = 0;
uint8_t currentAddress = 1;
bool found = false;
std::vector<uint8_t> validPorts;

void scani2cports()
{
namespace {
ScanState scanState = ScanState::IDLE;
uint8_t currentSDA = 0;
uint8_t currentSCL = 0;
uint8_t currentAddress = 1;
bool found = false;
uint8_t busyHits = 0;
std::vector<uint8_t> validPorts;

auto selectNextPort() -> bool {
currentSCL++;

if(validPorts[currentSCL] == validPorts[currentSDA]) currentSCL++;

if (currentSCL < validPorts.size()) {
Wire.begin((int)validPorts[currentSDA], (int)validPorts[currentSCL]); //NOLINT
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you have an overzealous linter active here. I'm getting zero complaints on this line.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one was about Wire specifically, and not including the header file for it directly. There isn't a good way I could think of that wouldn't also include a bunch of logic that's already defined in another header for that specific purpose, so I told it to ignore it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't specifically asking why the NOLINT was there, I was saying that I get no warnings there. This either tells me that you have an overzealous linter globally configured or that your environment isn't set up quite right.

Besides, what's wrong with pulling in the Wire lib? You already pulled in a bunch of standard headers. It wouldn't include any logic that isn't already in there if the code compiles fine.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because as far as I could tell, the different board versions have different Wire headers, and pulling in the wrong one would cause issues on a board it doesn't support. So instead of trying to find the easiest thing to pull in that makes misc-include-cleaner happy, I just told the linter to hush, because it does compile correctly since the logic to pull in the right version of the Wire lib does exist currently when compiling.

return true;
}

currentSCL = 0;
currentSDA++;

if (currentSDA >= validPorts.size()) {
if (!found) {
Serial.println("[ERR] I2C: No I2C devices found"); //NOLINT
}
#ifdef ESP32
Wire.end();
#endif
Wire.begin(static_cast<int>(PIN_IMU_SDA), static_cast<int>(PIN_IMU_SCL));
scanState = ScanState::DONE;
return false;
}

Wire.begin((int)validPorts[currentSDA], (int)validPorts[currentSCL]);
return true;
}
}

void scani2cports() {
if (scanState != ScanState::IDLE) {
return;
if (scanState == ScanState::DONE) {
Serial.println("[DBG] I2C scan finished previously, resetting and scanning again..."); //NOLINT
} else {
return; // Already scanning, do not start again
}
}

// Filter out excluded ports
for (size_t i = 0; i < sizeof(portArray); i++) {
if (!inArray(portArray[i], portExclude, sizeof(portExclude))) {
validPorts.push_back(portArray[i]);
}
}

validPorts.clear();
validPorts.reserve(portArray.size() - portExclude.size()); // Reserve space to avoid reallocations

for (const auto& port : portArray) {
bool isExcluded = false;

// Check if the port is in the excluded list
for (const auto& excluded : portExclude) {
if (port == excluded) {
isExcluded = true; // Port is excluded, break out of the loop
break;
}
}

if (!isExcluded) {
validPorts.push_back(port); // Port is valid, add it to the list
}
}

// Reset scan variables and start scanning
found = false;
currentSDA = 0;
currentSCL = 1;
currentAddress = 1;
scanState = ScanState::SCANNING;
}

bool selectNextPort() {
currentSCL++;
if(validPorts[currentSCL] == validPorts[currentSDA])
currentSCL++;
if (currentSCL < validPorts.size()) {
Wire.begin((int)validPorts[currentSDA], (int)validPorts[currentSCL]);
return true;
}

currentSCL = 0;
currentSDA++;
}

if (currentSDA >= validPorts.size()) {
if (!found) {
Serial.println("[ERR] I2C: No I2C devices found");
}
#ifdef ESP32
Wire.end();
#endif
Wire.begin(static_cast<int>(PIN_IMU_SDA), static_cast<int>(PIN_IMU_SCL));
scanState = ScanState::DONE;
return false;
}

Wire.begin((int)validPorts[currentSDA], (int)validPorts[currentSCL]);
return true;
}

void update()
{
void update() {
if (scanState != ScanState::SCANNING) {
return;
}

if (currentAddress == 1) {
#ifdef ESP32
if (currentAddress == 1) {
Wire.end();
}
#endif
}

Wire.beginTransmission(currentAddress);
byte error = Wire.endTransmission();
const uint8_t error = Wire.endTransmission();

if (error == 0)
{
if (error == 0) {
Serial.printf("[DBG] I2C (@ %s(%d) : %s(%d)): I2C device found at address 0x%02x !\n",
portMap[currentSDA].c_str(), validPorts[currentSDA], portMap[currentSCL].c_str(), validPorts[currentSCL], currentAddress);
found = true;
}
else if (error == 4)
{
Serial.printf("[ERR] I2C (@ %s(%d) : %s(%d)): Unknown error at address 0x%02x\n",
} else if (error == 4) { // Address was busy, log and warn
Serial.printf("[WARN] I2C (@ %s(%d) : %s(%d)): Busy at address 0x%02x, skipping !\n",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not necessarily busy, it can be, but error 4 specifically means that the I2C transaction couldn't be started, which can be caused by a lot of things.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, I went back into the files to get context on what error 4 was and it was commented to be busy, so that's what I used. I can change it to "Unable to start Transaction at (address)" instead

portMap[currentSDA].c_str(), validPorts[currentSDA], portMap[currentSCL].c_str(), validPorts[currentSCL], currentAddress);
busyHits++;
}

currentAddress++;

if (currentAddress <= 127) {
if (busyHits > 10) {
Serial.printf("[ERR] I2C: Too many busy hits (%d), power down tracker and check connections !\n", busyHits);
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this would be a great recommendation when it comes to official trackers for example

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically, with the official trackers, the connections would be the aux cable, not the IMU itself. If they don't have an aux cable and it throws this error, then it'll be a hardware fault. I'll add some preprocessor defines to make the official tracker message different than the DIY one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the main IMU works it won't trigger i2cscan, since the secondary IMU is marked optional

return;
}

currentAddress = 1;
selectNextPort();
}

bool inArray(uint8_t value, uint8_t* array, size_t arraySize)
{
for (size_t i = 0; i < arraySize; i++)
{
if (value == array[i])
{
auto inArray(uint8_t value, const uint8_t *array, size_t arraySize) -> bool {
for (size_t i = 0; i < arraySize; i++) {
if (value == array[i]) {
return true;
}
}

return false;
}

bool hasDevOnBus(uint8_t addr) {
auto hasDevOnBus(uint8_t addr) -> bool {
byte error;
#if ESP32C3
int retries = 2;
Expand Down Expand Up @@ -164,10 +191,10 @@ namespace I2CSCAN
* This code may be freely used for both private and commerical use
*/

int clearBus(uint8_t SDA, uint8_t SCL) {
#if defined(TWCR) && defined(TWEN)
auto clearBus(uint8_t SDA, uint8_t SCL) -> int {
#if defined(TWCR) && defined(TWEN)
TWCR &= ~(_BV(TWEN)); // Disable the Atmel 2-Wire interface so we can control the SDA and SCL pins directly
#endif
#endif

pinMode(SDA, INPUT_PULLUP);
pinMode(SCL, INPUT_PULLUP);
Expand Down
4 changes: 2 additions & 2 deletions lib/i2cscan/i2cscan.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ namespace I2CSCAN {
bool hasDevOnBus(uint8_t addr);
uint8_t pickDevice(uint8_t addr1, uint8_t addr2, bool scanIfNotFound);
int clearBus(uint8_t SDA, uint8_t SCL);
boolean inArray(uint8_t value, uint8_t* arr, size_t arrSize);
bool inArray(uint8_t value, const uint8_t *array, size_t arraySize);
}

#endif // _I2CSCAN_H_
#endif // _I2CSCAN_H_
2 changes: 2 additions & 0 deletions src/debug.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@
// disable if problems. Server does nothing with value so disabled atm
#define SEND_ACCELERATION true // send linear acceleration to the server

#define EXT_SERIAL_COMMANDS false // Set to true to enable extra serial debug commands

// Debug information

#define LOG_LEVEL LOG_LEVEL_DEBUG
Expand Down
26 changes: 25 additions & 1 deletion src/serial/serialcommands.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,19 @@
#include "nvs_flash.h"
#endif

#ifdef EXT_SERIAL_COMMANDS
#define CALLBACK_SIZE 8 // Increase callback size to allow for debug commands
#include "i2cscan.h"
#endif

#ifndef CALLBACK_SIZE
#define CALLBACK_SIZE 6 // Default callback size
#endif
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pattern is only useful if you expect the user to override this value, which we don't. If you are modernizing the code already, just use constexpr for this


namespace SerialCommands {
SlimeVR::Logging::Logger logger("SerialCommands");

CmdCallback<6> cmdCallbacks;
CmdCallback<CALLBACK_SIZE> cmdCallbacks;
CmdParser cmdParser;
CmdBuffer<256> cmdBuffer;

Expand Down Expand Up @@ -412,13 +421,28 @@ void cmdDeleteCalibration(CmdParser* parser) {
configuration.eraseSensors();
}

#if EXT_SERIAL_COMMANDS
void cmdScanI2C(CmdParser* parser) {
logger.info("Forcing I2C scan...");
I2CSCAN::scani2cports();
}
Comment on lines +425 to +428
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could probably go under "GET" instead


void cmdPing(CmdParser* parser) {
logger.info("PONG!");
}
#endif

void setUp() {
cmdCallbacks.addCmd("SET", &cmdSet);
cmdCallbacks.addCmd("GET", &cmdGet);
cmdCallbacks.addCmd("FRST", &cmdFactoryReset);
cmdCallbacks.addCmd("REBOOT", &cmdReboot);
cmdCallbacks.addCmd("DELCAL", &cmdDeleteCalibration);
cmdCallbacks.addCmd("TCAL", &cmdTemperatureCalibration);
#if EXT_SERIAL_COMMANDS
cmdCallbacks.addCmd("PING", &cmdPing);
cmdCallbacks.addCmd("SCANI2C", &cmdScanI2C);
#endif
}

void update() { cmdCallbacks.updateCmdProcessing(&cmdParser, &cmdBuffer, &Serial); }
Expand Down
Loading