Minor issues - Default I2C Address + Invoking enableInterrupt multiple times will lead to varying results

[lynniemagoo]

Peter,

The PCF8574A chips are harder to find these days. Should we consider changing the default I2C address to 0x20 which is the default address of PCF8574 and PCF8575? [EDIT] - I thought I remembered the default address being something other than 0x20. The only place this is located is in the README.MD with the example showing address as 0x38. I'm wondering now should we update the README.MD to show 0x38 for PCF8574A only as PCF8574 and PCF8575 both use 0x20 as the default I2C address.

Side-effects and use cases for enableInterrupt()

For both use cases here, one could say that these use cases relate to user error but based on writeup here, would like your feedback.

Consider this code from the module:

 /**
   * Enable the interrupt detection on the specified GPIO pin.
   * You can use one GPIO pin for multiple instances of the PCF857x class.
   * @param {number} gpioPin BCM number of the pin, which will be used for the interrupts from the PCF8574/8574A/PCF8575 IC.
   */
  public enableInterrupt (gpioPin: number): void {
    if (PCF857x._allInstancesUsedGpios[gpioPin]) {
      // use already initialized GPIO
      this._gpio = PCF857x._allInstancesUsedGpios[gpioPin];
      this._gpio['pcf857xUseCount']++;
    } else {
      // init the GPIO as input with falling edge,
      // because the PCF857x will lower the interrupt line on changes
      this._gpio = new Gpio(gpioPin, 'in', 'falling');
      this._gpio['pcf857xUseCount'] = 1;
      PCF857x._allInstancesUsedGpios[gpioPin] = this._gpio;
    }
    **this._gpio.watch(this._handleInterrupt);**
  }

Use Case #1 - EnableInterrupt called more than 1 time for same GPIO.

Fortunately, when GPIO unwatch is called, as the value for this._handleInterrupt is the same (is a bound function), the onoff package clears all listener entries matching the bound function so as long as disableInterrupt() is called, process will exit properly as all listeners were cleaned up.

myInstance1.enableInterrupt(17);
myInstance1.enableInterrupt(17);

//...
// all listeners are removed from whatever the current gpio is.
myInstance1.disableInterrupt();

One side-effect of this use case is that if enableInterrupt is called multiple times with the same pin number for gpio, when the interrupt does occur, this._handleInterrupt() is called multiple times to do a poll. Based on the design of the module, only the first poll() will fire changes, the 2nd poll() just chews up cpu with an unnecessary promise. In this case, the process can still exit as all listeners are indeed removed.

Use Case #2 - Could call this user stupidity where a single instance could be told to listen to interrupts for different GPIO pins (instance #1 listens to both GPIO17 and GPIO18). For this case, consider that only GPIO17 is used for PCF857X instance #1 and GPIO18 is used for instance #2. Should the user erroreously add GPIO 18 also to instance #1, you have 2 callbacks being executed (one for GPIO17 and one for GPIO18) and you will get false interrupt triggers.

myInstance1.enableInterrupt(17);
myInstance1.enableInterrupt(18);

//...
// all listeners are removed from whatever the current gpio object for #18 and as we are no longer tracking gpio17
// a listener is still registered there.
myInstance1.disableInterrupt();

Another possible side-effect not yet tested is whether the process can even exit as 'unwatch' was never called for GPIO17.

One way correct to this issue might be to invoke this.disableInterrupt() from within enableInterrupt execution. This would ensure that multiple callbacks are not invoked(Use case #1) and also ensure that processes can end properly if user registers single PCF857X instance with multiple GPIO instances. After further review of the disableInterrupt code, when the reference count goes to 0, a side-effect is that unexport is called for the gpio. Use Case #1 above for sure would break and Use Case#2 remains stable.

I shall think about this more in the coming days but wanted your thoughts on whether to even consider these 2 items.

Lyndel

[lynniemagoo]

OK. I found a way to make this work properly with minor changes. Each PCF857X instance can track the GPIO pin number (this._gpioPin) and use this to properly clean up previous registrations. I'd be happy to submit a small PR to correct this issue if you are open to reviewing. The changes I put in place should have 0 side-effects unless of course onoff does not let you recreate a GPIO instance on pinX in cases where the process previously did an unexport of the GPIO for pinX.

I will do additional testing around Use Case #1 above and if this has no issues, then I think my changes will be good.

Lyndel

[crycode-de]

Hi Lyndel,

thank you for your detailed explanations.

To the default address: I don't think there is a default I²C address for the ICs. The address is always defined be the three hardware address pins. Leaving the pins open may work but is likely to bring unexpected results. So the address is always user defined (even on the pre-soldered boards out there using jumpers).

The example address 0x38 listed in the readme and the examples originates from my custom hardware. 😄

To the possible multiple calls of enableInterrupt(pin): Hmm... it's not intended to call this again if the interrupt is already enabled. In my opinion it would be the best and cleanest way to check this._gpio first in the enableInterrupt method and throw an error if it is not null like this:

  /**
   * Enable the interrupt detection on the specified GPIO pin.
   * You can use one GPIO pin for multiple instances of the PCF857x class.
   * @param {number} gpioPin BCM number of the pin, which will be used for the interrupts from the PCF8574/8574A/PCF8575 IC.
   * @throws Error if the interrupt is already enabled.
   */
  public enableInterrupt (gpioPin: number): void {
    // check if gpio was enabled before
    if (this._gpio !== null) {
      throw new Error('GPIO already enabled');
    }
    // ...
  }

This would prevent multiple inits and possible overlaps and also inform the user about the mistake.

As this may be a breaking change (if a user did something wrong) this should increase the minor version to 3.1.0.

[lynniemagoo]

I can update my PR. Will do this this evening and resubmit. Sent from my iPhoneOn Apr 10, 2023, at 11:09 AM, Peter Müller @.*> wrote: Hi Lyndel, thank you for your detailed explanations. To the default address: I don't think there is a default I²C address for the ICs. The address is always defined be the three hardware address pins. Leaving the pins open may work but is likely to bring unexpected results. So the address is always user defined (even on the pre-soldered boards out there using jumpers). The example address 0x38 listed in the readme and the examples originates from my custom hardware. 😄 To the possible multiple calls of enableInterrupt(pin): Hmm... it's not intended to call this again if the interrupt is already enabled. In my opinion it would be the best and cleanest way to check this._gpio first in the enableInterrupt method and throw an error if it is not null like this: /

• Enable the interrupt detection on the specified GPIO pin.
• You can use one GPIO pin for multiple instances of the PCF857x class.
• @param {number} gpioPin BCM number of the pin, which will be used for the interrupts from the PCF8574/8574A/PCF8575 IC.
• @throws Error if the interrupt is already enabled. */ public enableInterrupt (gpioPin: number): void { // check if gpio was enabled before if (this._gpio !== null) { throw new Error('GPIO already enabled'); } // ... } This would prevent multiple inits and possible overlaps and also inform the user about the mistake. As this may be a breaking change (if a user did something wrong) this should increase the minor version to 3.1.0.

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: @.***>

[lynniemagoo]

I updated the examples with comments about the default address for each chip type. If you are good with those changes I will leave them in but change the enable interrupt accordingly. Sent from my iPhoneOn Apr 10, 2023, at 1:09 PM, Lyndel McGee @.> wrote:I can update my PR. Will do this this evening and resubmit. Sent from my iPhoneOn Apr 10, 2023, at 11:09 AM, Peter Müller @.> wrote: Hi Lyndel, thank you for your detailed explanations. To the default address: I don't think there is a default I²C address for the ICs. The address is always defined be the three hardware address pins. Leaving the pins open may work but is likely to bring unexpected results. So the address is always user defined (even on the pre-soldered boards out there using jumpers). The example address 0x38 listed in the readme and the examples originates from my custom hardware. 😄 To the possible multiple calls of enableInterrupt(pin): Hmm... it's not intended to call this again if the interrupt is already enabled. In my opinion it would be the best and cleanest way to check this._gpio first in the enableInterrupt method and throw an error if it is not null like this: /**

• Enable the interrupt detection on the specified GPIO pin.
• You can use one GPIO pin for multiple instances of the PCF857x class.
• @param {number} gpioPin BCM number of the pin, which will be used for the interrupts from the PCF8574/8574A/PCF8575 IC.
• @throws Error if the interrupt is already enabled. */ public enableInterrupt (gpioPin: number): void { // check if gpio was enabled before if (this._gpio !== null) { throw new Error('GPIO already enabled'); } // ... } This would prevent multiple inits and possible overlaps and also inform the user about the mistake. As this may be a breaking change (if a user did something wrong) this should increase the minor version to 3.1.0.

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: @.***>

[lynniemagoo]

OK. I had time to look at the code. I agree about adding the error but there's still an issue. Assume the following calls are made:

myInstance.enableInterrupt(13);
myInstance.disableInterrupt();
// This will be problematic and reuse an unexported GPIO as disableInterrupt() does not nullify element 13 of thePCF857x._allInstancesUsedGpios array.
myInstance.enableInterrupt(13);

So, I think the better approach would be to throw the error in enableInterrupt() as you suggested but we still need the cleanup code I added to disableInterrupt as there's no public method on the GPIO object in onoff to be able to get the original pin requested at construction, The current code implementation of onoff sets a private member field in the constructor. this._gpio = gpio;

I will do another PR later this evening with the hybrid changes.

[lynniemagoo]

PR updated. Please review.

[crycode-de]

Looks better, but I've added some remarks to your PR #56. Let's continue the discussion there. 😉