search

openhab / openhab1-addons

Add-ons for openHAB 1.x
Eclipse Public License 2.0
3.43k stars 1.69k forks source link

PiFace binding reads wrong state for output pins upon startup #2679

Closed sbjdk closed 9 years ago

sbjdk commented 9 years ago

The binding confuses input pins and output pins. Follow this to reproduce:

Simple items

Switch     PifaceSwitch0       "Switch 0" (all,piface) { piface="piface1:OUT:0" }
Switch     PifaceSwitch1       "Switch 1" (all,piface) { piface="piface1:OUT:1" }
Switch     PifaceSwitch2       "Switch 2" (all,piface) { piface="piface1:OUT:2" }
Switch     PifaceSwitch3       "Switch 3" (all,piface) { piface="piface1:OUT:3" }
Switch     PifaceSwitch4       "Switch 4" (all,piface) { piface="piface1:OUT:4" }
Switch     PifaceSwitch5       "Switch 5" (all,piface) { piface="piface1:OUT:5" }
Switch     PifaceSwitch6       "Switch 6" (all,piface) { piface="piface1:OUT:6" }
Switch     PifaceSwitch7       "Switch 7" (all,piface) { piface="piface1:OUT:7" }

Contact    PifaceContact0      "Contact 0" (all,piface){ piface="piface1:IN:0" }
Contact    PifaceContact1      "Contact 1" (all,piface){ piface="piface1:IN:1" }
Contact    PifaceContact2      "Contact 2" (all,piface){ piface="piface1:IN:2" }
Contact    PifaceContact3      "Contact 3" (all,piface){ piface="piface1:IN:3" }
Contact    PifaceContact4      "Contact 4" (all,piface){ piface="piface1:IN:4" }
Contact    PifaceContact5      "Contact 5" (all,piface){ piface="piface1:IN:5" }
Contact    PifaceContact6      "Contact 6" (all,piface){ piface="piface1:IN:6" }
Contact    PifaceContact7      "Contact 7" (all,piface){ piface="piface1:IN:7" }
  1. Switch on all PiFace outputs (e.g. using openHAB)
  2. Shutdown openHAB
  3. While holding down two of the physical input buttons on the PiFace, start up openHAB and let it initialize completely

Messages like these should appear in the log (INFO)

PifaceSwitch1 state updated to ON
PifaceContact1 state updated to CLOSED

When looking at the log and the UI, it becomes evident that the state of the output pins are not read correctly. All Switches are OFF - unless (as above) the input pin with the same number is CLOSED.

I suspect that the 'digital read' method should have BindingType (IN, OUT) included since both have pin numbers 0-7.

int value = sendDigitalRead(node, bindingConfig.getPinNumber());

teichsta commented 9 years ago

@sumnerboy12 could you probably have a look at this? Thanks, Thomas E.-E.

sumnerboy12 commented 9 years ago

@sbjdk I haven't used this binding for a couple of years now, so unfortunately I am not in a position to test/diagnose this. It sounds like you have figured out what the problem is however, are you able to create a pull request with your proposed changes?

sbjdk commented 9 years ago

Hi

I don't have a solution, but I would certainly like to contribute. Would you be available to answer some concrete questions in the process?

Thanks

/Steffen

2015-06-03 0:43 GMT+02:00 Ben Jones notifications@github.com:

@sbjdk https://github.com/sbjdk I haven't used this binding for a couple of years now, so unfortunately I am not in a position to test/diagnose this. It sounds like you have figured out what the problem is however, are you able to create a pull request with your proposed changes?

— Reply to this email directly or view it on GitHub https://github.com/openhab/openhab/issues/2679#issuecomment-108119196.

sumnerboy12 commented 9 years ago

If I can help I would be more than happy to oblige.

On 03/06/2015 7:43 p.m., sbjdk wrote:

Hi

I don't have a solution, but I would certainly like to contribute. Would you be available to answer some concrete questions in the process?

Thanks

/Steffen

2015-06-03 0:43 GMT+02:00 Ben Jones notifications@github.com:

@sbjdk https://github.com/sbjdk I haven't used this binding for a couple of years now, so unfortunately I am not in a position to test/diagnose this. It sounds like you have figured out what the problem is however, are you able to create a pull request with your proposed changes?

— Reply to this email directly or view it on GitHub https://github.com/openhab/openhab/issues/2679#issuecomment-108119196.

— Reply to this email directly or view it on GitHub https://github.com/openhab/openhab/issues/2679#issuecomment-108233286.

sbjdk commented 9 years ago

Hi Ben

There is a method read_output() in pfio that should help us. It should return a byte with the state of all 8 output ports.

https://github.com/thomasmacpherson/piface/blob/master/python/piface/pfio.py

I then looked at exposing this from ohpiface.py using a new command and found these command/ack identifiers in the code.

WATCHDOG_CMD = 14 WATCHDOG_ACK = 15

WRITE_OUT_CMD = 1 WRITE_OUT_ACK = 2 READ_OUT_CMD = 3 READ_OUT_ACK = 4 READ_IN_CMD = 5 READ_IN_ACK = 6 DIGITAL_WRITE_CMD = 7 DIGITAL_WRITE_ACK = 8 DIGITAL_READ_CMD = 9 DIGITAL_READ_ACK = 10

Some of these are not used. Do you see any reasons why WRITE_OUT, READ_OUT and READ_IN should not be implemented. READ_OUT, of course, is what I would use from the java side.

Thanks

/Steffen

2015-06-03 9:54 GMT+02:00 Ben Jones notifications@github.com:

If I can help I would be more than happy to oblige.

On 03/06/2015 7:43 p.m., sbjdk wrote:

Hi

I don't have a solution, but I would certainly like to contribute. Would you be available to answer some concrete questions in the process?

Thanks

/Steffen

2015-06-03 0:43 GMT+02:00 Ben Jones notifications@github.com:

@sbjdk https://github.com/sbjdk I haven't used this binding for a couple of years now, so unfortunately I am not in a position to test/diagnose this. It sounds like you have figured out what the problem is however, are you able to create a pull request with your proposed changes?

— Reply to this email directly or view it on GitHub <https://github.com/openhab/openhab/issues/2679#issuecomment-108119196 .

— Reply to this email directly or view it on GitHub https://github.com/openhab/openhab/issues/2679#issuecomment-108233286.

— Reply to this email directly or view it on GitHub https://github.com/openhab/openhab/issues/2679#issuecomment-108235399.

sumnerboy12 commented 9 years ago

Hi Steffen,

Just had a quick look at the ohpiface.py python script and from what I can tell it is not a simple change to do what you are asking, since the script doesn't 'know' what output pins it is responsible for. It is only configured with the list of pins to monitor.

The output pins are effectively 'late binding' - in that it is only when the script receives a write command for a given output pin, that is knows that pin is being controlled.

And because the input/output pins are numbered the same, you can't just read all output pins and publish their values back to the binding, since it will think they are the pins being monitored.

In short, the binding is pretty simple, and only really allows one-way comms with the output pins - you send commands and get an ACK that the command was received, but you can't monitor those pins.

Would probably be a welcome addition to change this, but would require a fair bit of re-working of the binding configuration in order to allow monitoring of these output pins somehow.

Sorry, thought it might be an easy fix but I am now getting recollections of looking at this when I wrote the binding and deciding it was going to be too tricky.

Ben

On 4/06/2015 9:19 a.m., sbjdk wrote:

Hi Ben

There is a method read_output() in pfio that should help us. It should return a byte with the state of all 8 output ports.

https://github.com/thomasmacpherson/piface/blob/master/python/piface/pfio.py

I then looked at exposing this from ohpiface.py using a new command and found these command/ack identifiers in the code.

WATCHDOG_CMD = 14 WATCHDOG_ACK = 15

WRITE_OUT_CMD = 1 WRITE_OUT_ACK = 2 READ_OUT_CMD = 3 READ_OUT_ACK = 4 READ_IN_CMD = 5 READ_IN_ACK = 6 DIGITAL_WRITE_CMD = 7 DIGITAL_WRITE_ACK = 8 DIGITAL_READ_CMD = 9 DIGITAL_READ_ACK = 10

Some of these are not used. Do you see any reasons why WRITE_OUT, READ_OUT and READ_IN should not be implemented. READ_OUT, of course, is what I would use from the java side.

Thanks

/Steffen

2015-06-03 9:54 GMT+02:00 Ben Jones notifications@github.com:

If I can help I would be more than happy to oblige.

On 03/06/2015 7:43 p.m., sbjdk wrote:

Hi

I don't have a solution, but I would certainly like to contribute. Would you be available to answer some concrete questions in the process?

Thanks

/Steffen

2015-06-03 0:43 GMT+02:00 Ben Jones notifications@github.com:

@sbjdk https://github.com/sbjdk I haven't used this binding for a couple of years now, so unfortunately I am not in a position to test/diagnose this. It sounds like you have figured out what the problem is however, are you able to create a pull request with your proposed changes?

— Reply to this email directly or view it on GitHub

<https://github.com/openhab/openhab/issues/2679#issuecomment-108119196 .

— Reply to this email directly or view it on GitHub

https://github.com/openhab/openhab/issues/2679#issuecomment-108233286.

— Reply to this email directly or view it on GitHub https://github.com/openhab/openhab/issues/2679#issuecomment-108235399.

— Reply to this email directly or view it on GitHub https://github.com/openhab/openhab/issues/2679#issuecomment-108618791.

sbjdk commented 9 years ago

Hi

I added this bit of code to ohpiface.py

elif packet.command == READ_OUT_CMD: if verbose: print "Digital read request for output pins" response = UdpPacket(READ_OUT_ACK, 0, pfio.read_output()) socket.sendto(response.for_network(), self.client_address)

Then I wrote a small Java application based on the sendCommand method from PifaceBinding. It sends READ_OUT_CMD and the byte that comes back holds the state of all 8 output pins. So far, so good.

I'm thinking that PifaceBinding could periodically issue this command. Any issues here that you can think of? I guess we would end up with calls like these:

eventPublisher.postUpdate(itemName, OnOffType.OFF); eventPublisher.postUpdate(itemName, OnOffType.ON);

/Steffen

2015-06-04 22:41 GMT+02:00 Ben Jones notifications@github.com:

Hi Steffen,

Just had a quick look at the ohpiface.py python script and from what I can tell it is not a simple change to do what you are asking, since the script doesn't 'know' what output pins it is responsible for. It is only configured with the list of pins to monitor.

The output pins are effectively 'late binding' - in that it is only when the script receives a write command for a given output pin, that is knows that pin is being controlled.

And because the input/output pins are numbered the same, you can't just read all output pins and publish their values back to the binding, since it will think they are the pins being monitored.

In short, the binding is pretty simple, and only really allows one-way comms with the output pins - you send commands and get an ACK that the command was received, but you can't monitor those pins.

Would probably be a welcome addition to change this, but would require a fair bit of re-working of the binding configuration in order to allow monitoring of these output pins somehow.

Sorry, thought it might be an easy fix but I am now getting recollections of looking at this when I wrote the binding and deciding it was going to be too tricky.

Ben

On 4/06/2015 9:19 a.m., sbjdk wrote:

Hi Ben

There is a method read_output() in pfio that should help us. It should return a byte with the state of all 8 output ports.

https://github.com/thomasmacpherson/piface/blob/master/python/piface/pfio.py

I then looked at exposing this from ohpiface.py using a new command and found these command/ack identifiers in the code.

WATCHDOG_CMD = 14 WATCHDOG_ACK = 15

WRITE_OUT_CMD = 1 WRITE_OUT_ACK = 2 READ_OUT_CMD = 3 READ_OUT_ACK = 4 READ_IN_CMD = 5 READ_IN_ACK = 6 DIGITAL_WRITE_CMD = 7 DIGITAL_WRITE_ACK = 8 DIGITAL_READ_CMD = 9 DIGITAL_READ_ACK = 10

Some of these are not used. Do you see any reasons why WRITE_OUT, READ_OUT and READ_IN should not be implemented. READ_OUT, of course, is what I would use from the java side.

Thanks

/Steffen

2015-06-03 9:54 GMT+02:00 Ben Jones notifications@github.com:

If I can help I would be more than happy to oblige.

On 03/06/2015 7:43 p.m., sbjdk wrote:

Hi

I don't have a solution, but I would certainly like to contribute. Would you be available to answer some concrete questions in the process?

Thanks

/Steffen

2015-06-03 0:43 GMT+02:00 Ben Jones notifications@github.com:

@sbjdk https://github.com/sbjdk I haven't used this binding for a couple of years now, so unfortunately I am not in a position to test/diagnose this. It sounds like you have figured out what the problem is however, are you able to create a pull request with your proposed changes?

— Reply to this email directly or view it on GitHub

<https://github.com/openhab/openhab/issues/2679#issuecomment-108119196 .

— Reply to this email directly or view it on GitHub

https://github.com/openhab/openhab/issues/2679#issuecomment-108233286.

— Reply to this email directly or view it on GitHub <https://github.com/openhab/openhab/issues/2679#issuecomment-108235399 .

— Reply to this email directly or view it on GitHub https://github.com/openhab/openhab/issues/2679#issuecomment-108618791.

— Reply to this email directly or view it on GitHub https://github.com/openhab/openhab/issues/2679#issuecomment-109043542.

sumnerboy12 commented 9 years ago

Hey Steffen,

That looks promising. I would try and avoid polling the RPi (or at least make it configurable so you can disable it if required) as I try to minimise this sort of continuous polling on my network.

But sending a request at startup to get the current state would be great. And then depending on the binding config you could periodically poll for changes as well.

Great work! Ben

teichsta commented 9 years ago

fixed by #2733 …