RoxMCP2301X & RoxLed: LED connected to MCP IC pin doesn't get updated via the ledControl() method

markuslang79 commented 2 years ago

There's a bug in the RoxLed.update() method while trying to control a LED connected to an MCP2301X IC using the MCP2301X.ledControl() method.

Here's an example code that turns a LED on and off twice using RoxLed followed by changing the MCP's LED_PIN directly using the digitalWrite method - afterwards it switches back to RoxLed again. For every toggle the state of the LED_PIN is printed before and after the toggle occurred using the MCP's digitalRead method. When using the RoxLed.toggle() method the state of the LED_PIN doesn't change:

platformio.ini

[env:esp32dev]
platform = espressif32
board = esp32dev
framework = arduino
monitor_speed = 115200
lib_deps = neroroxxx/RoxMux@^1.5.2

main.cpp

#define ROX_DEBUG

#include <Arduino.h>
#include <RoxMux.h>

static const uint8_t LED_PIN = 0;
static const ulong INTERVAL = 1000;
static const uint8_t MAX_COUNTER = 3;

RoxMCP23018<0x20> mcp;
RoxLed led;

static ulong lastToggleTime = 0;
static bool useRoxLed = true;
static uint8_t counter = 0;

void setup()
{
  Serial.begin(115200);
  while (!Serial) {;}
  Serial.println();
  delay(100);

  mcp.begin(true);
  mcp.pinMode(LED_PIN, OUTPUT);

  led.begin();
  led.setMode(ROX_DEFAULT);
}

void loop()
{
  mcp.update();
  mcp.ledControl(LED_PIN, led);

  ulong currentTime = millis();
  if (currentTime - lastToggleTime >= INTERVAL)
  {
    ROX_PRINTLN("-----");
    ROX_PRINTLN("BEFORE###   led pin:", mcp.digitalRead(LED_PIN));
    if (useRoxLed)
      led.toggle();
    else
      mcp.digitalWrite(LED_PIN, !mcp.digitalRead(LED_PIN));
    ROX_PRINTLN("AFTER###    led pin:", mcp.digitalRead(LED_PIN));

    lastToggleTime = currentTime;
    counter++;

    if (counter > MAX_COUNTER)
    {
      counter = 0;
      useRoxLed = !useRoxLed;
      ROX_PRINTLN("SWITCH###   use RoxLed:", useRoxLed);
    }
  }
}

serial output

rst:0x1 (POWERON_RESET),boot:0x13 (SPI_FAST_FLASH_BOOT)
configsip: 0, SPIWP:0xee
clk_drv:0x00,q_drv:0x00,d_drv:0x00,cs0_drv:0x00,hd_drv:0x00,wp_drv:0x00
mode:DIO, clock div:2
load:0x3fff0018,len:4
load:0x3fff001c,len:1044
load:0x40078000,len:10124
load:0x40080400,len:5828
entry 0x400806a8

-----
BEFORE###   led pin: 0
AFTER###    led pin: 0
-----
BEFORE###   led pin: 1
AFTER###    led pin: 1
-----
BEFORE###   led pin: 1
AFTER###    led pin: 1
-----
BEFORE###   led pin: 1
AFTER###    led pin: 1
SWITCH###   use RoxLed: 0
-----
BEFORE###   led pin: 1
AFTER###    led pin: 0
-----
BEFORE###   led pin: 0
AFTER###    led pin: 1
-----
BEFORE###   led pin: 1
AFTER###    led pin: 0
-----
BEFORE###   led pin: 0
AFTER###    led pin: 1
SWITCH###   use RoxLed: 1
-----
BEFORE###   led pin: 1
AFTER###    led pin: 1
-----
BEFORE###   led pin: 1
AFTER###    led pin: 1
-----
BEFORE###   led pin: 1
AFTER###    led pin: 1
-----
BEFORE###   led pin: 1
AFTER###    led pin: 1
SWITCH###   use RoxLed: 0
-----
BEFORE###   led pin: 1
AFTER###    led pin: 0
-----
BEFORE###   led pin: 0
AFTER###    led pin: 1
-----
BEFORE###   led pin: 1
AFTER###    led pin: 0
-----
BEFORE###   led pin: 0
AFTER###    led pin: 1

...

From my understanding the bug sits in the beginning of the RoxLed.update() method in line 112 where the !isOn() state is always consumed and returned with a value of 0 so that it's never able reach line 148 where a value of 1 should be returned in case of a stateChanged() has occurred.

This results in the RoxMCP2301X.ledControl() method to never get a cmd==1 value returned from the RoxLed.update() method so that the LED_PIN never get's pulled to LOW.

I'm not entirely familiar with the use cases of the RoxLed code since I only used it in combination with an MCP23018 IC so far - that's why I haven't created a pull request to fix this.

neroroxxx commented 2 years ago

Hi thank you for the heads up, you're right the bug is within RoxLed.h, i oversaw this bug because i used RoxLed mainly without the mcp to use the built in leds as a status indicator.

I will upload a fix as soon as im back on my computer. If you want to temporarily fix the issue just head over to the RoxLed.h file and update the following lines:

Line 114: this line should return 2 or 1 not 1 or 0, replace the 1 with 2 and 0 with 1
Line 134: same as line 114, replace the 1 with a 2 and the 0 with a 1
Line 145: replace the 0 with a 1

Basically the update method should return 0 when nothing has changed, 1 when the led has been turned off and 2 when the led is on.

Also since the led can blink or pulse, RoxLed may have the state of the led as ON but the actual led may be off since its blinking or pulsing so while the RoxLed is ON its state may be on or off, the update method returns where the actual led needs to be on/off or if it should not be changed (thats the 0 return)

This is what seems to be causing the issue with the mcp.

Let me know if that helps ill wire up a mcp20317 and test it

markuslang79 commented 2 years ago

Yes, I can confirm that your suggested changes do fix this issue for my use case.

Thanks for the quick response!

markuslang79 commented 2 years ago

Bug has been fixed with Version 1.5.3, thank you!

neroroxxx / RoxMux