added support for ads1015 #2231
Conversation
|
This appears to be a good extension of the current module, thanks a lot! Apart from adding support for ADS1015 chips, it also enables having more than one converter attached to the ESP. I see that you arranged for a descriptor array which covers up to four of them. That caught my interest as your implementation is now very close to an object-oriented approach. Except for the API, which acquired complexity with the I2C address selection parameter for each command. How about moving the API also towards objects, for example: conv1 = ads1115.setup(<i2c_id>, ads1115.ADDR_GND, ads1115.ADS1115)
conv2 = ads1115.setup(<i2c_id>, ads1115.ADDR_VDD, ads1115.ADS1015)
conv1:setting(<gain>, <dr>, ...)
conv2:setting(<gain>, <dr>, ...)
conv1:startread()
...
conv1:read()The You could even differentiate between the sub-types with dedicated constructors instead of the generic conv1 = ads1115.ads1115(<i2c_id>, ads1115.ADDR_GND)
conv2 = ads1115.ads1015(<i2c_id>, ads1115.ADDR_VDD)This would be very close to the approach we discussed in #2188. What do you think? |
|
@devsaurus, yes, the oop API looks better, particularly when esp32 and multiple i2c buses are considered. Is there a mechanism to share module code between 8266 and 32 branches? |
There isn't, sorry. |
|
I didn't squash to make move to oop API easier for review. |
|
How about testing artifacts? I have a couple of scripts which together with a hardware schematics could be checked in. |
Good decision, thanks.
This is/was discussed in #2145 -> no established mechanism yet. |
app/modules/ads1115.c
Outdated
| @@ -581,11 +557,23 @@ static const LUA_REG_TYPE ads1115_map[] = { | |||
| { LSTRKEY( "COMP_1CONV" ), LNUMVAL(ADS1115_CQUE_1CONV) }, | |||
| { LSTRKEY( "COMP_2CONV" ), LNUMVAL(ADS1115_CQUE_2CONV) }, | |||
| { LSTRKEY( "COMP_4CONV" ), LNUMVAL(ADS1115_CQUE_4CONV) }, | |||
| { LSTRKEY( "ADS1015"), LNUMVAL(ADS1115_ADS1015) }, | |||
| { LSTRKEY( "ADS1115"), LNUMVAL(ADS1115_ADS1115) }, | |||
| { LSTRKEY( "CMODE_TRAD"), LNUMVAL(ADS1115_CMODE_TRAD) }, | |||
| { LSTRKEY( "CMODE_WINDOW"), LNUMVAL(ADS1115_CMODE_WINDOW) }, | |||
| { LNILKEY, LNILVAL } | |||
There was a problem hiding this comment.
The __gc index is not defined - I suspect memory leaks when the device object is disposed by the garbage collector while timer_ref still holds a reference to the callback. Similarly, an armed os_timer would need to be shut down gracefully.
There was a problem hiding this comment.
Somehow I convinced myself that it will not happen, but on the second iteration I believe it will be worse than memory leak.
- Timer is armed.
- Device object is garbage collected.
- Timer fires, with ads_ctrl referencing user data which can be anything at this moment.
app/modules/ads1115.c
Outdated
| uint8_t i2c_addr = luaL_checkinteger(L, 1); | ||
| uint8_t i2c_id = luaL_checkinteger(L, 1); | ||
| if (i2c_id != 0) { | ||
| return luaL_error(L, "Invalid argument: i2c_id"); |
There was a problem hiding this comment.
It's recommended to use luaL_argcheck() when checking arguments.
I understand that you follow the pattern in the existing code, but unfortunately that was already inconsistent.
app/modules/ads1115.c
Outdated
| ads_ctrl_ud_t *ads_ctrl = luaL_checkudata(L, 1, metatable_name); | ||
| if (ads_ctrl->timer_ref != LUA_NOREF) { | ||
| os_timer_disarm(&ads_ctrl->timer); | ||
| lua_rawgeti(L, LUA_REGISTRYINDEX, ads_ctrl->timer_ref); |
There was a problem hiding this comment.
Why push timer_ref onto the stack? It's redundant at least and might leak memory since the return below says 0 results (not sure, can't run the code without a chip).
There was a problem hiding this comment.
A bit of cargo on my side - I took a snipped out of readoutdone without fully understanding what it does. Fixing it explained some other anomaly I have seen.
|
I gave a try to mispec mentioned in #2145. Here is the result: https://github.com/paweljasinski/nodemcu-mispec/blob/master/nodemcu/mispec_ads1115.lua |
* ads1015 is supported, up to 4 devices can be connected at the same time * removed debug, updated documentation * changed to oop API * added __gc to handle active timer cleanup * reworked argument validation and error reporting * stack is no longer messed up after __del
* ads1015 is supported, up to 4 devices can be connected at the same time * removed debug, updated documentation * changed to oop API * added __gc to handle active timer cleanup * reworked argument validation and error reporting * stack is no longer messed up after __del
Fixes #2223.
devbranch rather than formaster.docs/en/*.This PR introduces support for ADS1015 based on the code for ADS1115.
In addition this PR includes:
Due to formatting changes (removal of the tabs), it is probably good idea to use
?w=1when looking at the diff.