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 all commits
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
209 changes: 118 additions & 91 deletions lib/i2cscan/i2cscan.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,136 +1,163 @@
#include "i2cscan.h"
#include "../../src/globals.h"

#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"};
#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"};
#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};
#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};
#endif
#include <array>
#include <cstdint>
#include <string>

namespace I2CSCAN
{
enum class ScanState {
#include "../../src/globals.h"
#include "../../src/consts.h"

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;
namespace {
ScanState scanState = ScanState::IDLE;
uint8_t currentSDA = 0;
uint8_t currentSCL = 0;
uint8_t currentAddress = 1;
bool found = false;
uint8_t txFails = 0;
std::vector<uint8_t> validPorts;

#ifdef ESP8266
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)
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)
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, 5> portExclude = {12, 13, 16, 17, LED_PIN};
#elif defined(ESP32)
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

void scani2cports()
{
bool selectNextPort() {
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("[ERROR] 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;
}
template <uint8_t size1, uint8_t size2>
uint8_t countCommonElements(
const std::array<uint8_t, size1>& array1,
const std::array<uint8_t, size2>& array2) {

uint8_t count = 0;
for (const auto& elem1 : array1) {
for (const auto& elem2 : array2) {
if (elem1 == elem2) {
count++;
}
}
}

return count;
}
} // anonymous namespace

void scani2cports() {
if (scanState != ScanState::IDLE) {
return;
if (scanState == ScanState::DONE) {
Serial.println("[DEBUG] 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();
uint8_t excludes = countCommonElements<portArray.size(), portExclude.size()>(portArray, portExclude);
validPorts.reserve(portArray.size() - excludes); // Reserve space to avoid reallocations

for (const auto& port : portArray) {
if (std::find(portExclude.begin(), portExclude.end(), port) == portExclude.end()) {
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;
txFails = 0;
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)
{
Serial.printf("[DBG] I2C (@ %s(%d) : %s(%d)): I2C device found at address 0x%02x !\n",
if (error == 0) {
Serial.printf("[INFO ] 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) { // Unable to start transaction, log and warn
Serial.printf("[WARN ] I2C (@ %s(%d) : %s(%d)): Unable to start transaction at address 0x%02x!\n",
portMap[currentSDA].c_str(), validPorts[currentSDA], portMap[currentSCL].c_str(), validPorts[currentSCL], currentAddress);
txFails++;
}

currentAddress++;

if (currentAddress <= 127) {
if (txFails > 5) {
#if BOARD == BOARD_SLIMEVR_LEGACY || BOARD == BOARD_SLIMEVR_DEV || BOARD == BOARD_SLIMEVR || BOARD == BOARD_SLIMEVR_V1_2
Serial.printf("[ERROR] I2C: Too many transaction errors (%d), please power off the tracker and contact SlimeVR support!\n", txFails);
#else
Serial.printf("[ERROR] I2C: Too many transaction errors (%d), please power off the tracker and check the IMU connections!\n", txFails);
#endif
}

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])
{
return true;
}
}

return false;
}

bool hasDevOnBus(uint8_t addr) {
byte error;
#if ESP32C3
Expand Down Expand Up @@ -165,9 +192,9 @@ namespace I2CSCAN
*/

int clearBus(uint8_t SDA, uint8_t SCL) {
#if defined(TWCR) && defined(TWEN)
#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
21 changes: 20 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 7 // Increase callback size to allow for debug commands
#include "i2cscan.h"
#endif

#ifndef CALLBACK_SIZE
#define CALLBACK_SIZE 6 // Default callback size
#endif

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,23 @@ 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

#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("SCANI2C", &cmdScanI2C);
#endif
}

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