Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ESP32: improve gpio module #1612

Closed
2 of 4 tasks
jmattsson opened this issue Nov 27, 2016 · 11 comments
Closed
2 of 4 tasks

ESP32: improve gpio module #1612

jmattsson opened this issue Nov 27, 2016 · 11 comments

Comments

@jmattsson
Copy link
Member

jmattsson commented Nov 27, 2016

Rationale:
I've got a first-cut of the GPIO module done, which should be functional, but it's not complete. It's currently lacking in documentation. API differences so far:

  • All GPIO configuration now done via gpio.config(), which takes one or more tables each describing one or more GPIOs to set up a particular way. More on this below.
  • The gpio.serin()/gpio.serout() are not currently supported. I suspect they might be possible to implement much more efficiently using the I2S or RMT peripheral.
  • The trigger ("interrupt") callback now receives two arguments, the GPIO and the level. It could be argued that the arguments should be passed the other way around, but given we're not attempting to be NodeMCU8266 API compatible, the gpio,level way around seemed the cleaner.
  • Addition of gpio.wakeup() for configuring wake-from-sleep-on-GPIO-level functionality.

The GPIO configuration approach got an overhaul because I liked the way the IDF API does it, which essentially is bulk configuration of pins. It removes the tedium of setting up GPIOs, and I extended their model further in the Lua API to support multiple bulks. At this point we have:

gpio.config({
  gpio=x || {x, y, z},
  dir=gpio.IN || gpio.OUT || gpio.IN_OUT,
  opendrain= 0 || 1 -- only applicable to output modes
  pull= gpio.FLOATING || gpio.PULL_UP || gpio.PULL_DOWN || gpio.PULL_UP_DOWN
}, ...)

So, someone wanting two outputs and one input might run the single command:

gpio.config( { gpio={19,21}, dir=gpio.OUT }, { gpio=22, dir=gpio.IN, pull=gpio.PULL_UP })

Things to do:

  • Document the module.
  • The GPIO ISR needs to be moved to the platform layer, so that other modules/drivers can use GPIOs for interrupts. The module is now using the IDF per-pin ISR hook.
  • Possibly all the gpio_xyz() should be wrapped in the platform layer, but I suspect that might just be unnecessary pessimisation. We're not sharing a platform layer with any other chips here, so I'd be inclined to keep it mean and lean.
  • Consider gpio.serin()/gpio.serout() functionality.

Priority: Medium

Dependencies:

  • I2S or RMT driver for serin/serout.
@devsaurus
Copy link
Member

I studied gpio.trig to understand how to deal with interrupts in IDF.
The ISR service is installed with ESP_INTR_FLAG_IRAM and single_pin_isr() is consistently mapped with IRAM_ATTR. But the functions which get called aren't (gpio_intr_disable() and task_post_low()). I thought that all code that's executed within/from an ISR needs to be mapped IRAM_ATTR. Am I missing something?

@jmattsson
Copy link
Member Author

Hmm, it's quite possible I screwed something up when I had to change the gpio stuff to match the latest IDF when they did their gpio revamp. Do feel free to fix in whichever direction you reckon is best. We used to run from flash because that was the only option before. I don't really have an opinion either way as to whether the IRAM cost is worth the performance/responsiveness increase here.

@LDmitryL
Copy link

LDmitryL commented Mar 9, 2017

Hi, @jmattsson . Is it permissible to use the call writes the value to the port from trigger port the current implementation to ESP32?
gpio.config( { gpio=2, dir=gpio.OUT }); gpio.trig(0, 3, function(gpio, level) gpio.write(2,1) end );
this command causes:
PANIC: unprotected error in call to Lua API (stdin:1: attempt to index local 'gpio' (a number value))
And at the same time gpio.trig(0, 3, function(gpio, level) print("123") end ); works fine

@jmattsson
Copy link
Member Author

You're shadowing the global gpio when you're declaring your callback function to take the gpio, level parameters. Just rename that parameter to something else and you should be good.

@LDmitryL
Copy link

LDmitryL commented Mar 9, 2017

Thank you!!!
I forgot about this feature LUA

@devsaurus
Copy link
Member

@jmattsson I'm still not clear about the ISR requirements with respect to ISR code in IRAM. Missing a definite statement in some docs.
However, one way could be to force map the functions/*.o files to IRAM with an .ld script. Is there a way to include a nodemcu.ld somehow?

@jmattsson
Copy link
Member Author

It's actually quite a bit harder to get linker scripts going the way one might want to with the IDF, I'd recommend avoiding it when possible. The easy option in this case is to simply remove the ESP_INTR_FLAG_IRAM, at least for now. Or, if there aren't too many additional functions which would need to be marked with IRAM_ATTR, those could be so marked.

@devsaurus
Copy link
Member

Or, if there aren't too many additional functions which would need to be marked with IRAM_ATTR, those could be so marked.

I was thinking about linker magic because some of these functions are part of IDF's gpio driver. Guess we can't modify them and would need to re-implement them in an IRAMified copy.

@jmattsson
Copy link
Member Author

Ah, let's go with the easy option and not use the ESP_INTR_FLAG_IRAM then.

@devsaurus
Copy link
Member

Ok, done that.

@stale
Copy link

stale bot commented Jun 7, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jun 7, 2019
@stale stale bot closed this as completed Jun 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

No branches or pull requests

3 participants