platform_gpio_register_intr_hook does not update interrupt hook pins

[galjonsfigur]

While I was testing and cleaning up softuart module I discovered that serial receive from multiple pins does not work and only first registered object can receive. After some debugging I found that function platform_gpio_register_intr_hook should update interrupt hook but It doesn't do it. Because of that, other modules that uses this function (for example rotary module) also has this problem. I don't have any encodes around but is seems like only first registered rotary encoder would work (according to the documentation registering and using 3 encoders should be possible)

It seems like adding the following lines before the return here

      ETS_GPIO_INTR_DISABLE();
      // This is a structure copy, so interrupts need to be disabled
      platform_gpio_hook = oh;
      ETS_GPIO_INTR_ENABLE();

should fix this issue.

Expected behavior

platform_gpio_register_intr_hook should update interrupt hook

Actual behavior

platform_gpio_register_intr_hook does not update interrupt hook

Test code

rotary.setup(0,1,2,3)
rotary.setup(1,4,5,6)
-- Only the first registered one seems to work
rotary.on(0, rotary.ALL, function (type, pos, when)
  print "Position=" .. pos .. " event type=" .. type .. " time=" .. when
end)
rotary.on(1, rotary.ALL, function (type, pos, when)
  print "Position=" .. pos .. " event type=" .. type .. " time=" .. when
end)

NodeMCU startup banner

NodeMCU 3.0.0.0
branch: dev
commit: 88a33af
release: 2.0.0-master_20170202 +509
release DTS: 202005011052
SSL: false
build type: float
LFS: 0x0 bytes total capacity
modules: adc,bit,dht,file,gpio,i2c,mqtt,net,node,ow,rotary,softuart,spi,tmr,uart,wifi
build 2020-05-06 13:52 powered by Lua 5.3.5 on SDK 3.0.1-dev(fce080e)
cannot open init.lua:

Hardware

NodeMCU Devkit V0.9 with ESP-12E

[pjsg]

Good catch.

[galjonsfigur]

Forgot to ask - should I make a PR to fix it or leave it up to you @pjsg ?

[pjsg]

You should make a PR... Thank you

…

On Thu, May 7, 2020, 06:30 galjonsfigur ***@***.***> wrote: Forgot to ask - should I make a PR to fix it or leave it up to you @pjsg <https://github.com/pjsg> ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3089 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AALQLTMKZX3NVLWOYRPZJ73RQKETBANCNFSM4M2NRTOQ> .

[TerryE]

Having looked at this code, what this issue really exposes is that the approach to registering and registering interrupt hooks needs to have temporal update integrity because the GPIO ISR is currently concurrently to this update process.

Given that this is an very infrequent (typically zero or a few calls during startup) then the simplest and cleanest method of achieving this is to make platform_gpio_hook a pointer to the struct gpio_hook and append the gpio_hook_entry vector to this so that the gpio hook block is a single malloc unit. This really simplifies the code since the register routine can simply malloc the new block, fill it in, update platform_gpio_hook and then free the old one. Since word writes are atomic in fact we don't even need to mask GPIO interrupts.

This removes a lot of complexity rather than adding to it, which is a good thing, IMO.

Incidentally another issue is that this registration task can fail and so returns a boolean success status. However all of the current modules that do this registration rotary, NmraDcc, hx711 and softuart treat this as a void and assume success. 😬

PS: @galjonsfigur, I've rewritten this routine. It's quite a bit shorter now. I can send you the patch if others think that we should go this route.

[marcelstoer]

Sorry Terry, missed this comment. I just merged the PR (already approved by @pjsg ).

[TerryE]

@marcelstoer, that's fine. I can raise this as part of the module tidy up. As I said, this change doesn't really fix the underlying issue