-
Notifications
You must be signed in to change notification settings - Fork 51
Ina238 + 237 via a refactor of INA260 too. #765
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.
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
base: main
Are you sure you want to change the base?
Conversation
…eclare INA classes to hide enums from each other
@brentru This PR is ready for review, see notes above about the refactor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tyeth The refactor and splitting these drivers from .h to .cpp/.h makes sense to me. There are some syntactical things I'd like addressed before merging.
#include <Adafruit_Sensor.h> | ||
#include <Arduino.h> | ||
#include <Wire.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this added?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous chain of header includes get broken / ignored when switching scope to the cpp file, so as the includes from the ina2xx cpp call back up to this file it needs the Wire included to then refer to these twowire instances.
Just retested with it removed and platformIO fails to build. Letting CI run now to see if arduino likes it still. Then will reinstate #include <Wire.h>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Failed in https://github.com/adafruit/Adafruit_Wippersnapper_Arduino/actions/runs/16222764678/job/45807199135
Adding include back in a1cdc01
Ah damn, I've now overflowed some of the 4mb boards with the INA23x PR, so they'll need swapping to the no-ota partition sooner rather than later 😞 |
I'm going to temporarily migrate this to the no-OTA board repo, so we get the zip assets for <4MB boards |
This refactors the INA2xx components to avoid naming clashes in enums. Moving the driver includes and enum usage inside the cpp files keeps it scoped which avoids the conflicts between INA260 library and INA238/7/xx library.
We could probably do the same to resolve the naming conflict/redefined warnings from the vl ToF series (but that still builds).