added support for ads1015 #2231

paweljasinski · 2018-01-17T23:17:44Z

This PR is for the dev branch rather than for master.
This PR is compliant with the other contributing guidelines as well (if not, please describe why).
I have thoroughly tested my contribution.
The code changes are reflected in the documentation at docs/en/*.

This PR introduces support for ADS1015 based on the code for ADS1115.
In addition this PR includes:

support for up to 4 converters (hardware limit).
ads1115.setup() does not perform i2c general reset anymore. Reset is available as a separate function.

Due to formatting changes (removal of the tabs), it is probably good idea to use ?w=1 when looking at the diff.

devsaurus · 2018-01-18T22:38:07Z

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 <i2c_id> aims for future compatibility with the ESP32 which supports multiple I2C buses.

You could even differentiate between the sub-types with dedicated constructors instead of the generic setup():

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?

paweljasinski · 2018-01-21T22:15:35Z

@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?

marcelstoer · 2018-01-22T12:26:20Z

Is there a mechanism to share module code between 8266 and 32 branches?

There isn't, sorry.

paweljasinski · 2018-01-22T21:40:32Z

I didn't squash to make move to oop API easier for review.

paweljasinski · 2018-01-22T21:44:06Z

How about testing artifacts? I have a couple of scripts which together with a hardware schematics could be checked in.

marcelstoer · 2018-01-23T06:11:42Z

I didn't squash to make move to oop API easier for review.

Good decision, thanks.

How about testing artifacts?

This is/was discussed in #2145 -> no established mechanism yet.

devsaurus

Very nice job, thank you so much!

Please clarify the potential memory leak when gc'ing the object and this one's ready for merging.

devsaurus · 2018-01-24T21:29:57Z

app/modules/ads1115.c

@@ -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                                            }


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.

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.

devsaurus · 2018-01-24T21:36:37Z

app/modules/ads1115.c

-    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");


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.

devsaurus · 2018-01-27T19:42:13Z

app/modules/ads1115.c

+    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);


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).

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.

paweljasinski · 2018-01-28T00:28:27Z

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

paweljasinski added 2 commits January 18, 2018 00:03

ads1015 is supported, up to 4 devices can be connected at the same time

343c0bc

removed debug, updated documentation

ae9b625

marcelstoer added this to the 2.2 milestone Jan 20, 2018

marcelstoer added the new feature label Jan 20, 2018

changed to oop API

9a0ef4b

devsaurus requested changes Jan 24, 2018

View reviewed changes

devsaurus mentioned this pull request Jan 24, 2018

ads1115.setup() performs reset of all i2c devices #2238

Open

paweljasinski added 2 commits January 27, 2018 01:09

added __gc to handle active timer cleanup

09e64de

reworked argument validation and error reporting

10c5fc3

devsaurus reviewed Jan 27, 2018

View reviewed changes

stack is no longer messed up after __del

cb4006e

devsaurus approved these changes Jan 28, 2018

View reviewed changes

devsaurus merged commit f87d68f into nodemcu:dev Jan 29, 2018

devsaurus mentioned this pull request Jan 30, 2018

RFD - Structuring the sensor modules #2243

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added support for ads1015 #2231

added support for ads1015 #2231

paweljasinski commented Jan 17, 2018

devsaurus commented Jan 18, 2018

paweljasinski commented Jan 21, 2018

marcelstoer commented Jan 22, 2018

paweljasinski commented Jan 22, 2018

paweljasinski commented Jan 22, 2018

marcelstoer commented Jan 23, 2018

devsaurus left a comment

devsaurus Jan 24, 2018

paweljasinski Jan 25, 2018

devsaurus Jan 24, 2018

devsaurus Jan 27, 2018

paweljasinski Jan 28, 2018

paweljasinski commented Jan 28, 2018

added support for ads1015 #2231

added support for ads1015 #2231

Conversation

paweljasinski commented Jan 17, 2018

devsaurus commented Jan 18, 2018

paweljasinski commented Jan 21, 2018

marcelstoer commented Jan 22, 2018

paweljasinski commented Jan 22, 2018

paweljasinski commented Jan 22, 2018

marcelstoer commented Jan 23, 2018

devsaurus left a comment

Choose a reason for hiding this comment

devsaurus Jan 24, 2018

Choose a reason for hiding this comment

paweljasinski Jan 25, 2018

Choose a reason for hiding this comment

devsaurus Jan 24, 2018

Choose a reason for hiding this comment

devsaurus Jan 27, 2018

Choose a reason for hiding this comment

paweljasinski Jan 28, 2018

Choose a reason for hiding this comment

paweljasinski commented Jan 28, 2018