Repository navigation
Provide external-irq functionality directly in Pin class #1118
Description
Activity
I like this proposal, I can already see how it will reduce the code size of the CC3200 and I am always in the mood for that ;-)
One thing that I suggest is to pass to the callback function the pin object for which the callback is being triggered (that's how it is now implemented for the CC3200) instead of a "line number" like done on stmhal.
For the rest, the proposed API looks OK to me.I'm all in favor of this. The whole ExtInt thing seems a little unusual (as you mentioned most processors have some type of pin change interrupt).
I think for stmhal, the only thing I would do is to detect conflicts (so if you try to setup say pin B3 ad C3) and report them.
@dpgeorge should we give this a go? Using your proposed API and passing a Pin object to the callback?
My concern is that this tightly couples GPIO and IRQ handling. But IRQ handling is cross-device-block matter, how IRQs will be (are) handled for other components?
My inclination would be to make callback= just assign the callback and have the rising/falling attriute be separate (probably part of mode, or a separate parameter). I've seen cases where you want to change the riing/falling portion on the fly, and using a tuple for callback feels wrong to me.
@dhylands glad you are on board with this!
@danicampora yes, definitely we should pass the pin object to the callback. There is no concept of "line" anymore.
how IRQs will be (are) handled for other components?
In all cases they use the callback API proposed here: switch.callback, timer.callback, can.rxcallback. IRQ=callback and IRQs are, at least for stmhal, almost 1-1 correspondence with peripherals.
My concern is that this tightly couples GPIO and IRQ handling. But IRQ handling is cross-device-block matter, how IRQs will be (are) handled for other components?
It only couples irq handing for pin change to gpio. In the end, I think we want IRQ handling (in the form of callbacks) for pretty much every peripheral - uart, spi, can, i2c, timer, gpio. The thing that triggers the interrupt is often peripheral specific.
It only couples irq handing for pin change to gpio.
In fact, the way the ST HAL library is implemented, EXTI control is actually part of the GPIO module and there is no real way to decouple them in this case.
My inclination would be to make callback= just assign the callback and have the rising/falling attriute be separate (probably part of mode, or a separate parameter)
I think @dhylands has a good point there. Probably better to make the rising/falling (or other IRQ triggering mdoes, this is hardware specific) it part of the init, and also make it possible to change it on the fly.
have the rising/falling attriute be separate (probably part of mode, or a separate parameter)
Could do this. For a GPIO pin to be in EXTI mode it must be an input pin. So we can just add a Pin.INT_RISING mode.
@danicampora how does the CC3200 do it? Is EXTI exclusively input?
CC3200 does not have a separate EXTI module like the stm32. GPIO pins simply have the option to generate interrupts on level changes. The Pin needs to be an input of course.
Pin.INT_RISING
Maybe that is a good option. These are the modes the CC3200 has:
- Falling edge.
- Rising edge.
- Falling or rising.
- Low level.
- High Level.
In cases where you use the "level" modes instead of the "edges" then is important to be able to change the mode on the fly as @dhylands suggested.
We can make a branch with the new pin irq behaviour, code it all up to see how it works for the different ports, fine tune, then merge. Sound ok?
14 remaining items
- You could try making a general callback class that has a register address and bit number/mask so that the enable and disable methods can be generic and reused across all irqs.
Ok, good idea. Lets see how that turns out.
Sorry, I missed to comment previously:
To open up the discussion more, here is an alternative suggestion: a callback (irq line) is an object in its own right
To provide backwards compatibility with existing API could make the callback object accept the call method to get:So, what we have here is that trying to make it "simple" ("each object allows to set up callback"), it actually gets complex - "callback is an object", having its own state, usable in either "backwards compatible" or real way, etc. Besides being confusing, all this stuff is not going to fly with 16K heap. So, if there's interest in supporting such targets, that should be taken into account. The most simple and efficient abstraction is usually the one which captures target domain faithfully. If there's an interrupt controller in every microprocessor system, then exposing its functionality is the obvious way to let it be used, with further simplifications and user-friendly-helpers can be built on top of that. Btw, IRQController idea above is pretty much ExtInt generalized, cleaned up (ExtInt somehow deals with pull up/pull down) and with any state held in IRQController itself (no "ExtInt" objects).
Interesting... I am almost finished implementing the module to handle the CC3200's low power modes, and at the same time I took the chance to implement interrupt handling using the callback object approach that @dpgeorge proposed. I find it quite intuitive. It's also implemented in a way that there is only one "C interrupt helper" that takes care of calling all Python callbacks.
It works like this:sw = pyb.Pin('GPIO22', af=0, mode=pyb.Pin.IN, type=pyb.Pin.STD_PD) sw.callback(handler=swcallback, intmode=pyb.Pin.INT_RISING, priority=pyb.callback.LEVEL_2)
and of course:
cb=sw.callback() cb.init(handler=swcallback, intmode=pyb.Pin.INT_RISING)
to trigger the callback manually:
sw.callback()()
enable and disable:
sw.callback().enable() sw.callback().disable()
Of course the final naming has to be decided, but I think is a good approach. The way I implemented it every callback object takes 24 bytes of RAM. Is that too much?
"callback is an object", having its own state, usable in either "backwards compatible" or real way, etc. Besides being confusing, all this stuff is not going to fly with 16K heap
I did not write it above, but my plan was to make these callback objects all static/const/ROMable. Thus
pin.callbackdoes nothing more than get the address of the object. The only state you need in RAM (and you need it no matter how you implement it) is a pointer to the Python callback object (which ExtInt already has as a big array). All other configuration state (IRQ priority level, enabled/disabled, etc) is in the registers of the MCU.ExtInt somehow deals with pull up/pull down
But not in a clean way. ExtInt only works correctly for Pin objects. Even though it was intended for controlling other ExtInt sources (eg RTC alarm) it doesn't work, and this is because they don't all fit the same pattern.
The way I implemented it every callback object takes 24 bytes of RAM.
Sorry, I meant 8 bytes, not 24 ;-)
I couldn't immediately see any documentation about the IRQ internals in MicroPython. So I'm asking here, hope this isn't a bother.
I've written a custom MicroPython BuiltIn module for some hardware we are working with and I'd like to be able to invoke a MP function out of turn (essentially an IRQ, though it's not from a hardware IRQ). Should I use
mp_irq_queue_interruptto schedule the function to fire. Or can I call it directly withmp_call_function_*without expecting issues?Cheers.
You might want to consider
micropython.scheduledocs.Thanks for the reply @peterhinch
I did see the issues leading up to the implementation to
micropython.schedule, and while it looks promising, I'm not trying to invoke the function from Python space, but C space. Is this still the correct approach?I've a module and C code that runs in the
Serverstask. To update, I've tried both approaches I mentioned above. Both of which cause aLoadProhibitederror, though at different times.mp_call_function_1fails to invoke the python function at all.mp_irq_queue_interruptactually succeeds the first time, calling the python callback and having control return to my C code. On the second invocation the python callback is fired but then during the return it all falls down. AnotherLoadProhibited.Though I can't be sure yet, it looks like the stack perhaps gets corrupted, as the BackTrace didn't seem to look as I expected it.
If there is a better place to discuss this, I'll oblige.
I have no experience of doing this from C. It might be worth raising a query in the forum.
Thanks @peterhinch for your reply. Actually worked it out about half an hour ago. I'm tackling the Garbage Collector now. Unfortunately the memory is too fragmented for my use-case (We are running very close to the memory limits of our device, WiPy2). So I'll be attempting a Best-Fit/Worst-Fit GC implementation, hopefully that reduces fragmentation over the First-Fit implementation MicroPython uses now.
@nevercast I just redid allocations in CircuitPython to allocate qstrs, imports and types at the top of the heap. It appears to really help heap fragmentation (one test I did went had the longest free space go from 432 bytes to over 3400 bytes.) There is a before/after video of it here and the commit is here. It should be simple to patch into MicroPython and I'm happy to help with it. Want to open a new issue for it and we can discuss more there?
Now that we have a few ports to different MCUs, we can start to see some ways of making the functionality in the pyb module more MCU agnostic / more general. Eg see issue #1085.
I'd like to propose that the bulk of the ExtInt functionality be incorporated into the Pin class. In a general MCU a Pin object can generate an interrupt when the level changes. The pyboard supports this feature in a bit of a round-about way using ExtInt to configure the level changing external interrupt on a pin. This way of doing things does not map easily to the cc3200 or esp8266 ports, where it would make more sense to register the irq directly with a Pin object.
From a users point of view, I think being able to configure a pin directly for irq on level change is much easier that having to create a separate ExtInt object. Eg, new scheme would provide something like:
and/or you can set it in the init function directly:
Could also provide aux control such as
pin.trigger_callbackfor software irq trigger,pin.disable_callbackandpin.enable_callback.Main benefits are:
The stmhal port can still provide ExtInt for finer control, but this will be mostly unnecessary I think. It has support for non-pin irqs (eg RTC alarm), but actually this is not working properly and when I've been trying to implement proper RTC wakeup, I've anyway needed to rewrite specific ext-int support. Also, the fact that ExtInt actually calls HAL_GPIO to configure the irq means that it's much more suited to be part of the Pin class.
Any thoughts? Any suggestions for the API to setup and control the pin irq?