PCF8574 Inputs stop responding when other pins used as output.

[Budman1758]

Steps to reproduce

How can we trigger this problem? Set up a PCF8574 with 4 outputs and 4 inputs. Set up 4 as input switches as 4 tasks. Or 3 as in my setup.

Does the problem presist after powering off and on? (just resetting isnt enough sometimes) Yes

Expected behavior

When powered up at first inputs respond as they should. I have 2 reed switches to simulate a door open and closed position indicator. When each switch is actuated I have a rule that writes a line on an OLED display. Works great until ANY of the outputs are triggered.

Actual behavior

When 1 output is triggered 1 of the 2 inputs stops responding. When you trigger another output the second input stops responding. Only way to get the inputs to respond again is to power cycle the setup.

System configuration

-01 module. PCF8574 IO expander. OLED display module (I2C) SI7102 temp and humidity sensor. 4 channel relay board. Temp sensor and OLED module all work fine. Ports 1-4 set as inputs and ports 5-8 connected to relays. 3 tasks for 3 switch inputs to PCF8574.

This is consistent and repeatable with 3 or 4 different PCF8574 chips I have and have tried several different "late model" ESP releases. Right now using >> GIT version: | mega-20180111

I thought I was imaging this but its VERY consistent. After messing around with this for the last 2 days I'm afraid I have no more hair to pull out. Wasn't much to start with tho.....

Let me know what you need as to screenshots or configs.

BTW. The logic is reversed on this chip. It takes a "0" to get a positive output. That normal?? Seems kinda backwards. That had me going for a while......

[clumsy-stefan]

when you look at P019 (PCF8574) it does the following:

case PLUGIN_TEN_PER_SECOND:

  {
    int state = Plugin_019_Read(Settings.TaskDevicePort[event->TaskIndex]);
    if (state != -1)
    {
      if (state != switchstate[event->TaskIndex])
      {
        String log = F("PCF  : State ");
        log += state;
        addLog(LOG_LEVEL_INFO, log);
        switchstate[event->TaskIndex] = state;
        UserVar[event->BaseVarIndex] = state;
        event->sensorType = SENSOR_TYPE_SWITCH;
        sendData(event);
      }
    }
    success = true;
    break;
  }

As I understand this part, it means, that whenever a PIN that is defined as an input in a task is changing it's value, it should generate an event and update the respective value.

This seems to work partially. Especially when using low-level-triggered relays. but it seems to fail with hig-level-triggered SSR's.

What I think is strange is, that when you do a explicit "status,pcf," you get the correct value as a status back. but obviously the ten_per_second function does not see this and does not update the value....

I'm still trying to debug what exactly happens, but couldn't get to the cause yet... so any ideas / pointers are welcome!

[TD-er]

Hmm looking at that code makes my code senses tingle. Look at this beauty in the Plugin_019_Write function: if(!getPinState(PLUGIN_ID_019, unit, &mode, &value) || mode == PIN_MODE_INPUT || (mode == PIN_MODE_OUTPUT && value == 1)) Just try to decide for yourself what's going on here. It may very well be valid and good working code, but I see a lot of potential errors in that line and it takes me minutes to decipher it. So in my opinion it is bad code, because you cannot read it and get confidence in knowing it is correct.

For example, looking at the function getPinState does only update the value when the combination plugin and index is known in pinStates. So this line will set the corresponding portmask bit high when one of the previous checks (this is checked in a for-loop) yielded a value of "1" and some mode == PIN_MODE_OUTPUT and also when the combination of plugin and index is unknown. I don't know if that's meant to be like this, but this also happens when all pinStates are occupied. (PINSTATE_TABLE_MAX = 32) I don't see any function that may clear such a claimed pinstate.

The hardwareInit function already claims 17 of these and binds them to plugin 1.

You asked for some pointers to look at. :) My first attempt would be to see how many pinStates entries are left with plugin set to 0.

[clumsy-stefan]

ok, thanks... I'll have a look... but as it's not my plugin, and I currently don't know who has written it, I'll have to start from scratch o understand... as you say, some lines are difficult to read although most likely correct....

[TD-er]

The state of "pinStates" is also something that I am not sure about. It is one update which is easy to forget to do and also easy to miss, since updates are done per bit. So it is very well possible this cache can get out of sync. (also its fixed size may cause issues) That's why I want a proper central GPIO handler to deal with that.

[uzi18]

@TD-er this is my code: if(!getPinState(PLUGIN_ID_019, unit, &mode, &value) || mode == PIN_MODE_INPUT || (mode == PIN_MODE_OUTPUT && value == 1)) we get state of pin and check:

• if not in pin state table so we assume as input -> we pullup
• if input - we pullup
• if output and setted to 1 - we pullup

what's wrong with that?

[clumsy-stefan]

@uzi18: I use the plugin a lot. I don't think it's wrong, I just see a different behaviour between low-level-trigger output's and high-level-triggered output's.

Just as I idea what I'm trying to acheive: I'd like to get an active feedback if the state of a pin changes, no matter if it's an input or output.. Eg. if a pin is changed from a rule (for example because timer kicked in) I'd like to get a feedback to the controller. to do this I define the pin as "switch"... When using low-level triggered relays that seems to work fine. when using high-level-active relais it does not work. I assume becuase the "normal" state is 0 and therefore the pin is always assumed to be an output pin...

why not just read the effective pin state in the ten_per_second call and compare it to the stored state. and if it's changed generate an event (trigger the relevant task), no matter if the pin is already defined as input or output...

sorry fro my english, but I hope you understand what I mean...

[uzi18]

This is only problem with pinstate API - it is more C-like, have used here what was available. But we can update getPinState to return enum like PIN_UNKNOWN,PIN_INPUT,PIN_OUTPUT_0,PIN_OUTPUT_1 On the other hand why we trying to find problem when real problem is somewhere else?

My question is: do we realy need to generate event when output change state?

[clumsy-stefan]

if you want to act on it on a controller side, yes, then you need an actual state... or if you want to display the actual state. I use it in my home-automation, so I wan't to see in the GUI the acutal state of the GPIO/Output without polling it, even when it's changed "locally" (eg from a rule). also the commands that can be used are abviously different (eg. on when it's off and vice versa).

[uzi18]

@clumsy-stefan @TD-er see here big space for improvement - we use in plugins local variables with switchstate/outputstate etc. and also we have global get/set/hasPinState...etc.

[uzi18]

@clumsy-stefan how do you change state of pcf gpio for this high trigger SSR?

[TD-er]

this is my code: if(!getPinState(PLUGIN_ID_019, unit, &mode, &value) || mode == PIN_MODE_INPUT || (mode == PIN_MODE_OUTPUT && value == 1)) we get state of pin and check:

if not in pin state table so we assume as input -> we pullup if input - we pullup if output and setted to 1 - we pullup what's wrong with that?

The mode and value are being set and used in the same statement, that's what makes it very hard to "prove" it to be correct. If the function returns false, the change of these values doesn't matter, but if it is true, then these values have been changed. Also if the function returns false, it may be false not because it wasn't set, but because there is no more space left in the pinstates array. I don't like these constructs, because errors in there are very easy overlooked and it is rather hard to imagine what really happens in there. I didn't say it is non-working code, it is just that I cannot read it at normal speed and know from experience lines like that may take hours/days to spot if they do contain the error. When using multiple || or &&, please assign them to some const bool to indicate what's being checked. And if (... check + update variable) else { ... do process changed values... }

It is just that code that cannot be read with ease is useless code, since code is read more often than written. (at least it should be) No matter how ingenious the code is. You probably also know code like "that's written by ... and nobody understands what it does, better not touch it. It has been working well for ages." That's -in my view- useless code, since you cannot (re)use it.

So that's what could be better, but that's just my opinion. But on the other hand, you will not see me write code like *pointer++. It may be valid code and very normal among programmers, but it is just something that takes some time to think about what is happening. So maybe I am just too cautious about that.

[clumsy-stefan]

@uzi18: when the unit starts I set all relevant PIN's to 0 with pcfgpio,,0 after this I set the state with pcfgpio,, with 0 for off and 1 for on for

[uzi18]

@TD-er have understand what You trying to say, we can add there some comments to be more readable for now. We also need to think about moving all common gpio stuff into core, so all root problems with gpio could be fixed in one place, don't know if it will be goal for v2.0 or v2.1.

@clumsy-stefan Is it possible for you to measure voltage on such pcf output pin connected to ssr input? measure voltage next change state to 1 - ssr on. measure one more time next change state to 0 - ssr off measure one more time for me it is hardware related problem, not plugin

[TD-er]

@uzi18

We also need to think about moving all common gpio stuff into core, so all root problems with gpio could be fixed in one place, don't know if it will be goal for v2.0 or v2.1.

I totally wholeheartedly agree. If time permits and it doesn't appear to be too much of a snake pit when opened, I want to do that before the next release.

[clumsy-stefan]

@uzi18: sure I can when I'm on site again, however I think it's very unlikely that it's a hardware issue, because when I read the status of the output with "status,pcf," I get the correct value!! Typically SSR's are 0-1.5V off, 1.5-2.5V undefined and 2.5-5V on (for high level triggered, and vice versa for low level triggered).

[uzi18]

@clumsy-stefan please power up pcf8574 with 5V.

[clumsy-stefan]

@uzi18: yes, always! VCC for pcf, relais, SSR and D1 mini are always stabilized 5V.

As written, when requesting the status via the 'status' command of the plug-in I get the actual correct state!

[uzi18]

have test hardware now with ssr high level, what is your voltage for 1 and 0 ?

[clumsy-stefan]

I don't have physical address currently to the unit but I don't understand the question really. When I set the pcf to 0 the SSR shuts off, if I set the pcf to 1 the SSR turns on (connected lamp lights up). Do the physical connection send to work! Also if I request the status from the pcf I get the correct value back. Only the plug-in does not trigger an event when changing the output! I guess that's an (intended) software logic... It was not meant to trigger events on output pin changes!

[uzi18]

Ok I'm working on this ...

[clumsy-stefan]

cool, thx... I also looked at the code, but can't get my head around it currently... let me know if I can help or test!

[clumsy-stefan]

@uzi18: one more thing I just realised, the plugin is defined as "SENSOR_TYPE_SINGLE" but switches (as GPIO) should have "SENSOR_TYPE_SWITCH" so the controller (plugins) recognize them correctly! Probably also related to the above mentioned problem...

EDIT: I guess all extra IO Modules (MCP, PCF, etc.) should redefine this?!

[giig1967g]

The state of "pinStates" is also something that I am not sure about. It is one update which is easy to forget to do and also easy to miss, since updates are done per bit. So it is very well possible this cache can get out of sync. (also its fixed size may cause issues)

@uzi18 I have been looking into the mcp & pcf plugin and don't understand the reason for this global variable: "pinState". I mean I see what it does but I wonder if it is really needed. In PCF plugin for example it is read only twice with getPinState: once in the plugin_write and one in pcfgpiotoggle command (that I wrote... but that I can easily change)

Shouldn't we remove it to avoid synch issues?

And what do you think about switching to this library that is already compatible with interrupts: https://github.com/skywodd/pcf8574_arduino_library

[TD-er]

I've also been looking at this pinState array and I think it is a bad design waiting for bugs. You always have to keep it in sync. See also https://github.com/letscontrolit/ESPEasy/issues/1593

[uzi18]

@TD-er @giig1967g I was using this to track mode and status to resolve some problems and only because it was in API. We need something like that.

[uzi18]

@giig1967g this library is designed for AVR mcu.

[giig1967g]

@TD-er @giig1967g I was using this to track mode and status to resolve some problems and only because it was in API. We need something like that.

Please consider this a brainstorming session :) Why instead of using a global variable (pinStatus) we don't use a local variable for keeping trace only of the mode status? As a matter of facts, we already store the value inside switchstatus. We might just need a switchmode[ ] array of bytes for PCF, GPIO and MCP. the advantages could be: a) It will use max 12x3=36bytes (only if the 3 plugin are enabled) instead of 32x5=160bytes b) it will remove the potential problems of having to synch switchstatus with pinState... c) it will avoid all the loops (32 times) that are defined inside getPinState and the loops (32 or even 64 times) inside setPinState.

@uzi18 @TD-er what do you think?

[TD-er]

I have to look at this in the morning. My personal favor is to have some object oriented approach, since you then can scale out without the need of adding extra pre-allocated memory allocations which you probably always estimate the wrong size. On the other hand, handling a lot of bit states is screaming for bitwise operators and masks to do a lot of pins in just one clock tick. (OR/XOR/AND)

So I have to read this topic again tomorrow before making up my mind on this :)

[uzi18]

@giig1967g pinStatus is used also on webserwer subpage to show state of gpios

problem is that every plugin has got some variables with something[TASK_MAX], even if it is only 1 element in use. also PCF/MCP plugins should cover up to 4 pins per task max - grouped by configuration, now it is waste of tasks.

for sure we need something unified to store actual state of gpios

[giig1967g]

@uzi18 I know. I have deeply studied the code to understand the logic. One thing it's still not clear to me. Maybe you can help me understand.

Why in the PLUGIN_INIT the setPinState sets the state to "0" indipendently from the actual status? _P019_PCF8574.ino - line 183: setPinState(PLUGIN_ID_019, Settings.TaskDevicePort[event->TaskIndex], PIN_MODE_INPUT, 0);

Same code is present in P001 (Switch-line 215) and P009 (MCP-line 186)

[uzi18]

Don't ask me it is Your commit, it were never before here ;)

[uzi18]

schould be not 0 but switchstate[event->TaskIndex] or UserVar[event->BaseVarIndex] if we want also inverted value there

[giig1967g]

Don't ask me it is Your commit, it were never before here ;)

Oops...You are right. I must have copied it and pasted from P009 (MCP). Will fix it

[tonhuisman]

This seems to be solved, so can be closed.