Cleanup I2Cscan library #459
Conversation
| { | ||
| enum class ScanState { | ||
| namespace I2CSCAN { | ||
| enum class ScanState : uint8_t { |
There was a problem hiding this comment.
This isn't pushed through the network, why give it a defined underlying type?
There was a problem hiding this comment.
Other than not needing the full 4 bytes of space to do what 1 byte could?
There was a problem hiding this comment.
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.
| if(validPorts[currentSCL] == validPorts[currentSDA]) currentSCL++; | ||
|
|
||
| if (currentSCL < validPorts.size()) { | ||
| Wire.begin((int)validPorts[currentSDA], (int)validPorts[currentSCL]); //NOLINT |
There was a problem hiding this comment.
I think you have an overzealous linter active here. I'm getting zero complaints on this line.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
src/serial/serialcommands.cpp
Outdated
| #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 |
There was a problem hiding this comment.
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
| void cmdScanI2C(CmdParser* parser) { | ||
| logger.info("Forcing I2C scan..."); | ||
| I2CSCAN::scani2cports(); | ||
| } |
There was a problem hiding this comment.
This could probably go under "GET" instead
lib/i2cscan/i2cscan.cpp
Outdated
| if (busyHits > 10) { | ||
| Serial.printf("[ERR] I2C: Too many busy hits (%d), power down tracker and check connections !\n", busyHits); | ||
| } | ||
|
|
There was a problem hiding this comment.
Not sure if this would be a great recommendation when it comes to official trackers for example
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
If the main IMU works it won't trigger i2cscan, since the secondary IMU is marked optional
|
I'm not sure what happened locally but not the tracker won't boot unless I send a reboot command. Moving this to a draft until I can figure that out. Also, cleaning up the changes and stuff and getting it ready to push to my remote to make sure it compiles correctly |
|
Okay, the code now compiles correctly, it's formatted correctly, and I haven't had the issue pop up with the tracker needing to be rebooted since the last few commits. I'm assuming its something funky with my setup that's causing it (Linux). Think I'm ready for round 2 Also, I apologize for the fault in the formatting of the code in my commits. Those responsible have been sacked. Mynd you, møøse bites Kan be pretti nasti... |
This is a simple cleanup of the I2Cscan library.
It wasn't the best formatted, and seemed like a good candidate for me to start tinkering with the firmware and learn how it works. I modernized the code, and it's functionally identical to the older code. It also throws a lot less linter warnings, and it's a bit more exact with possible issues.
I also added a way to send debug commands over the serial connection, if the tracker is configured to accept those commands. They default to disabled, but setting
EXT_SERIAL_COMMANDSto true indebug.hwill enable them. I used this to manually trigger I2C scans for testing, and thought it was useful enough to submit it upstream.