Driver for INA3221 current and power and bus voltage monitor#12055
Driver for INA3221 current and power and bus voltage monitor#12055maribu merged 3 commits intoRIOT-OS:masterfrom
Conversation
|
I want to uncrustify it just before it gets merged. |
maribu
added
Area: drivers
|
It seems the return value of |
maribu
added
the
Reviewed: 3-testing
maribu
left a comment
maribu
left a comment
There was a problem hiding this comment.
Looks pretty good in general and works reliable for me. I have some suggestions to improve further:
- Move
static inlinefunctions fromdrivers/ina3221/ina3221.ce.g. todrives/ina3221/include/ina3221_internal.h. That way you can include that file from the test and move the test code guarded by an#ifdef ... #endiffromdrivers/ina3221/ina3221.cto the driver - Some functions of the user facing API can be made a bit easier to use without increasing ROM size, by adding
static inlinefunctions as wrapper in front of the implementation. E.g. that could be used to create a bitmask in the static inline function, which is than passed to the implementing function. (See inline comments for more details) - The use of submodules could make the code a bit cleaner, by moving the code guarded by
#ifdef MODULE_INA3221_WITH_ALERTSto its own file - Some style issues need to be fixed
- No use of CamelCase (e.g. use
int32_t in_uvorint32_t in_u_vinstead ofint32_t in_uV) - White space errors (e.g. trailing new line). The CI (travis) will tell you about this, or you can check for this by running
make static-testin the root of the Repo. But beware: This will rebase your code on top of master. So run e.g.git checkout -b tmp-for-static-testto create a new temporary branch thatmake static-testcan mess up, and that can be removed later. - Missing spaces e.g. between
ifand(. You can useuncrustifyto address these.
- No use of CamelCase (e.g. use
maribu
left a comment
maribu
left a comment
There was a problem hiding this comment.
See inline comments regarding internal pull ups.
|
Rebase, uncrustify and squash? |
|
Yes |
fabian18
force-pushed
the
driver-ina3221
branch
from
10caddd to
901c1c9
Compare
fabian18
force-pushed
the
driver-ina3221
branch
2 times, most recently
from
f2e66b5 to
644967b
Compare
|
I guarded the alert module members in ina3221_t with a preprocessor statement to save some ROM if the module ina3221_alerts is not used. |
|
Tested successfully with one sensor using SAUL in the default example, BOARD= |
| .name = | ||
| ina3221_saul_info[i * INA3221_SAUL_DRIVERS_NUM + j].name, | ||
| .driver = | ||
| &ina3221_saul_drivers[i * INA3221_SAUL_DRIVERS_NUM + j] |
There was a problem hiding this comment.
| &ina3221_saul_drivers[i * INA3221_SAUL_DRIVERS_NUM + j] | |
| &ina3221_saul_drivers[j] |
There was a problem hiding this comment.
Oh yes, because every device has the same SAUL driver. But the same applies for the driver name.
There was a problem hiding this comment.
It is not the driver name, but the device names. An end user adding more entries to ina3221_params will also have to add the same number of names to ina3221_saul_info.
(If the names would be reused, the user would have trouble to tell the devices apart.)
drivers/ina3221/ina3221_saul.c
Outdated
| }, | ||
| (saul_driver_t) { | ||
| .read = read_shunt_voltage_sum, | ||
| .write = saul_notsup, |
There was a problem hiding this comment.
I'm not sure if this is a good idea, but I just drop this here to let you decide on this:
How about adding a write function to control the enabled channels. E.g. you could say that 1 (== 0b001) is channel 1, 2 (== 0b010) is channel two, and 4 (==0b100) is channel three. If I would write 0, all channels would be disabled for the sum. Writing 5 (== 0b101) would enable 1 and 3, but not channel 2. etc.
There was a problem hiding this comment.
I think this is a great idea. I will add this soon.
|
EDIT: Tested version 0598711 - works partially: There should be The This custom params file was used to configure for two sensors: |
This is expected. See comment above. Once that is addressed, we should finally be able to merge :-) |
fabian18
force-pushed
the
driver-ina3221
branch
from
6ac8075 to
feae100
Compare
|
Tested feae100 - works completely: Ran the |
|
Can you rename the commit message of the last commit? Something like |
maribu
added
Reviewed: 1-fundamentals
fabian18
force-pushed
the
driver-ina3221
branch
from
feae100 to
94ad802
Compare
done |
maribu
added
Reviewed: 5-documentation
|
The build of the test failed on some boards for two reasons:
|
fabian18
force-pushed
the
driver-ina3221
branch
from
94ad802 to
8ab0526
Compare
|
I tried to compile for all boards in |
|
I see in Murdock, the build process still fails for esp8266 boards. |
maribu
left a comment
maribu
left a comment
There was a problem hiding this comment.
Travis found some spelling issues. Also some #includes use the wrong format, see inline comments
tests/driver_ina3221/main.c
Outdated
| #include <periph/gpio.h> | ||
| #include <xtimer.h> |
There was a problem hiding this comment.
Those are RIOT specific, and should use quotes (e.g. #include "xtimer.h")
tests/driver_ina3221/main.c
Outdated
|
|
||
| #include <periph/gpio.h> | ||
| #include <xtimer.h> | ||
| #include "string.h" |
There was a problem hiding this comment.
This one is provided by the standard c lib, it should be #include <string.h>.
The difference between <> and "" in the include is that with "" #include will first check the local folder for headers with that name, and then will fall back to the system header paths (e.g. those provided by the standard c lib). With <> the search will start directly in the system header paths.
Usign "" instead of <> risks that a same named file in the local folder is accidentally included, when the intention was to include a system header.
| #define INA3221_PARAMS { \ | ||
| .i2c = INA3221_PARAM_I2C, \ | ||
| .addr = INA3221_PARAM_ADDR, \ | ||
| .pin_warn = INA3221_PARAM_PIN_WRN, \ | ||
| .pin_crit = INA3221_PARAM_PIN_CRT, \ | ||
| .pin_tc = INA3221_PARAM_PIN_TC, \ | ||
| .pin_pv = INA3221_PARAM_PIN_PV, \ | ||
| .gpio_config = (INA3221_PARAM_INT_PU_PIN_WRN << INA3221_ALERT_WRN) | \ | ||
| (INA3221_PARAM_INT_PU_PIN_CRT << INA3221_ALERT_CRT) | \ | ||
| (INA3221_PARAM_INT_PU_PIN_TC << INA3221_ALERT_TC) | \ | ||
| (INA3221_PARAM_INT_PU_PIN_PV << INA3221_ALERT_PV), \ | ||
| .config = INA3221_PARAM_CONFIG, \ | ||
| .rshunt_mohm = { \ | ||
| INA3221_PARAM_RSHUNT_MOHM_CH1, \ | ||
| INA3221_PARAM_RSHUNT_MOHM_CH2, \ | ||
| INA3221_PARAM_RSHUNT_MOHM_CH3 \ | ||
| } \ |
There was a problem hiding this comment.
uncrustify messed up the alignment of the back slashes here. (The tool cannot be configured to get this right.) Can you fix this manually? They should be placed on a column name that is a multiple of 4; unlike other indents which should be 4*n+1.
|
So the compiler for esp8266 does not seem to accept it if members of an anonymous struct are initialized directly. |
|
@gschorcht: Maybe you could have a look why the initialize is not accepted by the ESP8266 toolchain? It seems to work with every other toolchain including the ESP32 (where it not only builds but also runs fine). |
|
I can compile it with ESP8266 toolchain without any problem. Murdock is also happy. |
|
Oh, sorry. @fabian18 fixed it that instant I wrote you. Sorry for the fuzz! |
|
No problem. |
fabian18
force-pushed
the
driver-ina3221
branch
2 times, most recently
from
da189b7 to
7e8cce8
Compare
|
🎉 |
|
Thank You |
5 participants
Contribution description
This PR adds support for the 3-channel current and bus voltage monitor INA3221.
The driver is integrated into SAUL as well.
Testing procedure
I have used a nucleo-f446re and an INA3221 breakout board with three 100 mOhm shunt resistors.
Test with test application:
I have used a yellow, a green and a blue LED.
make flash BOARD=<your_board>andmake term BOARD=<your_board>.Test with SAUL
cd examples/defaultUSEMODULE=ina3221 make flash BOARD=<your_board>andmake term BOARD=<your_board>saulto see SAUL commandsIssues/PRs references
None