add mcp23017 Lua module

[plomi-net]

Fixes #3057

Make sure all boxes are checked (add x inside the brackets) when you submit your contribution, remove this sentence before doing so.

• 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/*.

Add MCP23017 port expander support as a lua module. See issue #3057 and old pull request #3067
(rebase doesn't working correctly, sry)

[sonaux]

Please add mcp23017_example.lua from previous PR.

[plomi-net]

Please add mcp23017_example.lua from previous PR.
yes sure, sry

[galjonsfigur]

@plomi-net Travis build is failing because of luacheck warnings. I've looked and it seems like you can just tell luacheck to ignore them by adding -- luacheck: no unused to lines 129 and 151 at the end of them

[galjonsfigur]

I decided to make a review for this PR and I've newer done any before, so I'm sorry if the review isn't as good as it should be. I have an MCP23017 somewhere so I will try to test the hardware itself in near future.

[plomi-net]

@galjonsfigur thank you for the advice with Travis build check. It is good to know. I refactor the function so the check was passed now.

[galjonsfigur]

Thanks for your help. I hope you mean this error:

PANIC: unprotected error in call to Lua API (script4.lua:65: attempt to concatenate a nil value)

I added an condition to catch this error.

Yes - that one. I think the module is ready to be merged. The only thing that I could to is to add the note about untested possibility to work with MCP23018 IC, but other than that LGTM. Thanks for the work on this!

[nwf]

I'm not jazzed about this printing and then always returning a table (that just happens to have deviceOK set to false on the case of an error). I would prefer that checkAddress and checkDevice be moved to the example code and setup removed from the driver code. This has the added benefit that all the deviceOK checks can also be dropped.

[TerryE]

A few comments:.

1. The return value from require"mcp23017": whilst this can be anything (eg. a function or userdata), the convention is that "well behaved" modules should return a table; and other types such as function (as is this case) are unconventional. We might want to do this on an ESP, if the table form offers little benefit, as using a direct function saves a little RAM. Even so, this should be clearly documented as this is non-standard usage, and it could confuse many Lua developers.
2. The old throw vs. return status debate. Historically the initial NodeMCU module developers tended to avoid use of throw and (usually) return status -- which the application program often ignores leading to all sorts of bizarre silent errors. This was largely because uncaught errors panicked the ESP and restarted the processor with no error diagnostics. This is no longer the case. It is also not the Lua way, and the more that I've worked with this language and platform, the more I believe in the wisdom of sticking to Lua practice. IMO, hard errors such as using the wrong address, channel or pin indicate some coding error and should just be thrown; return statuses should only be used for soft / potentially intermittent errors.
3. So for example checkAddress() and checkDevice() are only called once during setup, but just throwing an error makes the code simpler. If you don't want the entry function to throw then why not just wrap the self:setup() in a pcall? In the case of deviceOK, this is set up true or false only at setup. Currently setup still returns an object with deviceOK set to false, but any subsequent call fails. So why have this status at all? Why not just return nil or throw an error if the device isn't OK and only return the object if it is OK. This way all the deviceOK checks are redundant since the functions can only be called if it is OK. And since you are using a function constructor return from the require , there also seems little point in exposing setup as a public method.
4. checkPinIsInRange() should definitely throw an error rather than just returning a nil as your code doesn't check this return value isn't checked, and this is just going to cause bizarre behaviour in readByte(), etc.
5. A related issue is using "print" statements for error diagnostics. This is great if you are watching your device with a UART plugged in, but many usecases will be headless. Having printed errors is a total PITA. At least if you throw the error, then the application can capture it with a pcall and the application can choose to print it, log it or return it over the WiFi.
6. This is a matter of style, but sometimes the Lua Ternary Operator form makes code simpler and shorter. For example getDirRegisterAddr() could be coded as return bReg == self.GPB and MCP23017_IODIRB or MCP23017_IODIRA, or even in this case return bReg and MCP23017_IODIRB or MCP23017_IODIRA.
7. Using metatables or not. There are pros and cons here. In some cases their use is practically essential: if you are using C modules and ROTables. If your objects will be mutli-instance in most cases, etc., because the one metatable is shared across all instances. However, they can also have distinct drawbacks: for example for k,v in pairs(mcp) do print(k,v) end does not print out your methods. I feel that if you are going to the extra level of complexity of using a metatable then it is best to do this properly. In this case all R/O properties should be in the metatable and not the instance table, e.g. INPUT, OUTPUT, GPA, GPB, HIGH and LOW.
8. An another example of this avoidable complication is that routines like readbyte() are in essence low level but you are making them class methods to you've got lots of self table accesses in them. This adds code size and runtime. Why not pass the address and i2cid in as parameters? This allow you to bind these as lazy closures. So, let's assume that you keep the metatable.

local isINPUT, isGBP, isHIGH = true, true, true -- use upvals in inline code to avoid self lookups

-- The metatable initialisation becomes
local mcp23017 = {
  __index = mcp23017,  
  INPUT = isINPUT,   OUTPUT = not isINPUT,
  GPA   = not isGBP, GPB    = isGPB, 
  HIGH  = isHIGH,    LOW    = not isHIGH
}

-- Below is an example of how we could strip down the redundancy.  The other 
-- functions could either be left as class methods acquring state from self
-- or move to lazy closures with the state also passed in as upvals.

return function(address, i2cId)
  local self = setmetatable({}, mcp23017)
  if setup(self, address, i2cId) then 
    self.writeIODIR = function(self, bReg, newByte)
       writeByte(address, i2cId,                         -- upvals
                 bReg == isGBP and MCP23017_IODIRB or MCP23017_IODIRA, newByte)
    end

    self.writeGPIO = function(self, bReg, newByte)
      writeByte(address, i2cId,                          -- upvals
                bReg == isGBP and MCP23017_GPIOB or MCP23017_GPIOA, newByte)
    end

    self.readGPIO = function(self, bReg)
      readByte(address, i2cId,                           -- upvals
               bReg == isGBP and MCP23017_GPIOB or MCP23017_GPIOA)
    end
    return self
  end
  return nil
end

Anyway, most style-type comments are a matter of personal preference. This is intended mostly as food for thought rather than mandatory requests, but get back to me if you think that I am talking out of my arse or you don't understand my points. 😄

[marcelstoer]

@plomi-net Do you plan to address Terry's comments?

[plomi-net]

@plomi-net Do you plan to address Terry's comments?

@marcelstoer Sorry, time passes quickly. I would like to respond to the comments from Terry, which are really helpful. I had already fixed a few little things.

[plomi-net]

@TerryE and @nwf Thank you for your detailed comments.
I have optimized some parts and that saves about 80 lines of code.
I agree with most of the comments.

One of your first comments is that a function is returned instead of a table.
This is still the case, I suppose. I am a bit confused about the Lua wisdom.

Furthermore, there are certainly many advantages and disadvantages to using a metatable or allowing multi instances. The MCP23017 can theoretically be used with 8 devices that need different instances.

I look forward to your feedback and thank you for your help.

[galjonsfigur]

New version of the code works like it should (tested quickly on a breadboard with DIP version of MCP23017) It takes considerably less RAM (at least when being run from flash and not using LFS). I will test how it behaves on LFS and how much RAM usage differs between current and previous version of the module.

[galjonsfigur]

The neat thing about making function that after being called returns table with all the module functions and constants is that running the module from LFS is extremely simple and requires changing line or two of code - in this particular case from:

local mcp23017 = require "mcp23017"
...
local mcp = mcp23017(address, i2cId)

to

local mcp = node.LFS.get("mcp23017")()(address, i2cId)

The difference between the previous and current version of the module in case of RAM usage when in LFS is about 100 bytes less with current version - for me a clear improvement!

[plomi-net]

@galjonsfigur an improvement of 100 Bytes is amazing. How much RAM does the program use now?