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

py: Add support for soft interrupt handlers. #2081

Closed
wants to merge 2 commits into from

Conversation

@HenrikSolver
Copy link
Contributor

commented May 15, 2016

In #1788 I made a proof of concept for soft IRQs, now I have finally got some time to do it properly. There are still things to do such as tests and documentation, but I would like the code to be reviewed.
I have also added support for soft interrupts in the CAN driver to be able to test the code properly. So with this patch one can do things like this.

import machine
from pyb import CAN
from time import sleep

bus1 = CAN(1, CAN.LOOPBACK)
bus1.setfilter(0, CAN.MASK16, 0, (0, 0, 0, 0))

bus2 = CAN(2, CAN.LOOPBACK)
bus2.setfilter(0, CAN.MASK16, 0, (0, 0, 0, 0))


def softcb(b, r):
    print("*Soft irq")
    s = "*We can allocate memory, yay!"
    print(s)
    if b.any(0):
        message = b.recv(0)
        print("*id:" + str(message[0]))
        print(message)

def hardcb(b, r):
    print("*Hard irq")
    print(b)
    machine.setsoftirq(softcb, b, r)

bus1.rxcallback(0, softcb, soft=True)
bus2.rxcallback(0, hardcb)

def go():
    v=0

    for x in range(10):
        bus1.send("hej",x)
        #Just some code to keep the VM running
        for y in range(1000):
            v=v+x
        print(str(x) + ' ' + str(v))

    print("\nTesting softint while sleeping")
    print("Sending message")
    bus1.send("mess1",1)
    bus1.send("mess2",2)
    sleep(1)
    print("Awake again!\n")

    print("And then doing it the hard way")
    bus2.send("hardmess",3)
    sleep(1)

Import the code as a module and then call go().

Things to discuss.

  • Should this functionality be configurable or included in all ports?
  • Should the pending exception stuff be converted to a soft IRQ? Something about this were mentioned by @dhylands in the discussions around #1788
@HenrikSolver HenrikSolver force-pushed the HenrikSolver:softirq2 branch 4 times, most recently from 55cf4d4 to aa9e721 May 15, 2016
@HenrikSolver

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2016

@dpgeorge, is there any interest in this? Currently I have some time that I can spend adding this feature to more classes.

@dpgeorge

This comment has been minimized.

Copy link
Member

commented Jun 1, 2016

Yes this is a good start. I want to see something like this in stmhal, but even better if it can be a general feature for all ports to use.

The main thing is to work out how to make it efficient. I see that you have replaced the check for a pending exception with a general check for a pending anything (could be a soft interrupt or an exception). So I'd say it doesn't add any overhead which is great!

It might be good to support soft priorities, but that can be added later.

Should this functionality be configurable or included in all ports?

We should try to make it available for other ports to use if they want.

Should the pending exception stuff be converted to a soft IRQ?

What do you mean by this?

@HenrikSolver

This comment has been minimized.

Copy link
Contributor Author

commented Jun 1, 2016

We should try to make it available for other ports to use if they want.

As it looks now, it is a small thing to enable the functionality in a port. The absolute majority of the work is to add it to every class.

What do you mean by this?

When I made this PR I had an idea that the mp_pending_exception in pendsv.c could be generalized to a soft interrupt. But since then I have been experimenting a bit, and I don't think it is a great idea any longer. Just forget that comment.

@dpgeorge

This comment has been minimized.

Copy link
Member

commented Jun 2, 2016

Ok, well let's try this feature on pyboard for now and if it proves successful we can generalise to to other ports.

A good peripheral to apply it to next would be external pin interrupts.

@HenrikSolver HenrikSolver force-pushed the HenrikSolver:softirq2 branch 2 times, most recently from 1cb09c0 to 7db1b81 Jun 4, 2016
@HenrikSolver HenrikSolver force-pushed the HenrikSolver:softirq2 branch 3 times, most recently from 456e3b4 to a13c731 Jun 15, 2016
@HenrikSolver HenrikSolver force-pushed the HenrikSolver:softirq2 branch from a13c731 to 0d00399 Jul 6, 2016
@HenrikSolver

This comment has been minimized.

Copy link
Contributor Author

commented Jul 6, 2016

@dpgeorge can you have a look at this? I have added support for soft irqs in extint and usrsw modules in additions to the initial CAN module. One thing should be mentioned, since the VM is not running when in the REPL soft callback functions are not executed in that mode. This code can be used as a starting point for experimenting with the user switch. The machine_pinbase test is failing on the pyboard, but I get the same result on master branch so I assume that I have not messed it up.

import pyb
import time

def cb():
    message = 4.5 * 3.2
    print(message)


sw = pyb.Switch()
sw.callback(cb, soft=True)

for x in range(10):
    print(x)
    time.sleep(2)
@HenrikSolver HenrikSolver force-pushed the HenrikSolver:softirq2 branch from 0d00399 to 2aa2647 Sep 10, 2016
@@ -247,7 +251,7 @@ long SpiWrite(unsigned char *pUserBuffer, unsigned short usLength)

unsigned char ucPad = 0;

// Figure out the total length of the packet in order to figure out if there

This comment has been minimized.

Copy link
@pfalcon

pfalcon Sep 18, 2016

Contributor

Such changes are better be avoided.

This comment has been minimized.

Copy link
@HenrikSolver

HenrikSolver Sep 19, 2016

Author Contributor

I blame my editor, it is set to remove trailing whitespace.

@@ -103,7 +103,7 @@ void mp_deinit(void) {
//mp_obj_dict_free(&dict_main);
mp_module_deinit();

// call port specific deinitialization if any

This comment has been minimized.

Copy link
@pfalcon

pfalcon Sep 18, 2016

Contributor

Ditto.

@pfalcon

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2016

I eyeballed over this PR, and didn't record my comments, which isn't first time and always regret. Trying to remember what I found then.

@@ -1256,12 +1256,27 @@ unwind_jump:;

pending_exception_check:
MICROPY_VM_HOOK_LOOP
#if MICROPY_PY_SOFTIRQ
if (MP_STATE_VM(mp_pending_ex_flags)) {

This comment has been minimized.

Copy link
@pfalcon

pfalcon Sep 18, 2016

Contributor

From this passage it's clear that there's no need to introduce mp_pending_ex_flags - we already have MP_OBJ_SENTINEL which can be stored in any mp_obj_t.

This comment has been minimized.

Copy link
@pfalcon

pfalcon Sep 18, 2016

Contributor

Ok, maybe it's needed, if both exception and softirq can occur asynchronously.

This comment has been minimized.

Copy link
@HenrikSolver

HenrikSolver Sep 19, 2016

Author Contributor

I think they can.


mp_obj_t mp_add_softint(mp_obj_t function, mp_obj_t arg1, mp_obj_t arg2) {

if (sp < MAX_STACK_DEPTH - 1 && softirq_enabled) {

This comment has been minimized.

Copy link
@pfalcon

pfalcon Sep 18, 2016

Contributor

If the idea is that softirq's should faithfully emulate real irq, "&& softirq_enabled" doesn't seem to be correct. How real IRQs work is that if they asserted, they are pending ("go in queue"), then if and when they're enabled, they're actually executed. Not recording IRQs when they're disabled thus means losing them.

This comment has been minimized.

Copy link
@HenrikSolver

HenrikSolver Sep 19, 2016

Author Contributor

Soft IRQs is only disabled when in REPL and when the softirq stack has been overflowed. The reason is that the VM is not running when in REPL which means that any soft IRQs are not consumed which means that the softirq stack will be overflowed.

This comment has been minimized.

Copy link
@pfalcon

pfalcon Sep 20, 2016

Contributor

Well, then again, that's not how "real" IRQs work. And it's worth considering what are the requirements for SoftIRQ implementation. An obvious one would: work as close as possible to HardIRQs, except allow to execute arbitrary code in a handler. Making them work with REPL should be doable, at least REPL already checks for pending exceptions, so can check for pending SoftIRQs either.

This comment has been minimized.

Copy link
@dpgeorge

dpgeorge Sep 20, 2016

Member

I agree with @pfalcon: there should be a fix number of slots, one for each distinct soft irq, and the slot is activated when the soft irq is raised. This means there is no way to overflow the pending irq list. But it does mean you need to decide what the classes of irqs there are (but I guess that they are the same number as the hard irqs).

This comment has been minimized.

Copy link
@pfalcon

pfalcon Sep 20, 2016

Contributor

@dpgeorge : I guess you mean the idea that SoftIRQs should follow HardIRQs in behavior closely. I'm not sure about the exact implementation though. From common sense, HardIRQ handler indeed can just set a single-bit flag in some variable for SoftIRQ, just the same as CPU sets that bit in some register for HardIRQ to happen. But our API and latency requirement may may it not that simple. For example, we attach handlers not to hardware IRQ numbers, but to specific objects, right? For example, to a specific Pin, which may share IRQ eith other pins. And with SoftIRQ processing delays, the same HardIRQ may happen again, but for another object, while first wasn't yet served. So, having a proper queue, with callback params as done by @HenrikSolver makes sense. (I don't even ask why there're slots for up to 2 args, it also seems to be optimized, up to support a handler which is a method of object and accepts a callback arg (though, that worth to be checked ;-) )) .

This comment has been minimized.

Copy link
@pfalcon

pfalcon Sep 20, 2016

Contributor

With hard irqs there is only ever 0 or 1 outstanding irq for any given irq handler, and that's the behaviour I think soft irq should have as well.

Yes, but latency of processing is such that 1 outstanding is usually adequate. With SoftIRQ, with explicitly introduce much larger latency, so may need to take some counter-measures to not lose too many IRQs.

This comment has been minimized.

Copy link
@HenrikSolver

HenrikSolver Sep 20, 2016

Author Contributor

In my mind a softirq can never behave as a real hard irq, so I don't try to do that. Instead I see a softirq as a kind of exception in the VM. As @pfalcon writes, it is quite easy to get more than one event of the same kind queued. Just the bounces of the user switch is enough to overflow the buffer if the depth is set to two. Regarding the number of arguments stored for each slot, it is actually used for the CAN receive callbacks where the first argument is the CAN object and the second argument is which of three possible reasons for the callback (Message received, receive buffer full and receive buffer overflowed).

This comment has been minimized.

Copy link
@dpgeorge

dpgeorge Sep 21, 2016

Member

Regarding the number of arguments stored for each slot, it is actually used for the CAN receive callbacks where the first argument is the CAN object and the second argument is which of three possible reasons for the callback (Message received, receive buffer full and receive buffer overflowed).

For hard irqs the reason for the callback is supposed to be stored in the irq object itself, accessible via .flags() method; see https://github.com/micropython/micropython/wiki/Hardware-API#irqs for an example of how that works. The same thing should be done for soft irqs, and that requires making a soft irq object.

This comment has been minimized.

Copy link
@HenrikSolver

HenrikSolver Sep 21, 2016

Author Contributor

Yes, the preferred hardware/IRQ API has changed since the CAN stuff were developed. I guess that it should be updated. I feel that I have some responsibility to do that :-)

This comment has been minimized.

Copy link
@dpgeorge

dpgeorge Sep 22, 2016

Member

The API is still somewhat in flux, so might be better to wait a bit until it's settled down/decided. But what shouldn't change is the fact that irq handlers will only take 1 argument (being the peripheral). Well, they could arguably take 2 arguments: the peripheral and the irq object but I think it's better to keep handlers as simple/minimal/fast as possible, ie only 1 argument.

@pfalcon

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2016

So, besides inline comments:

While working on esp8266 port, @dpgeorge introduced MICROPY_VM_HOOK_* facility. It allows to plug into VM loop on the source level without patching it, and could be used to implement softirq facility cleaner.

General issue with such implementation is latency - it takes to wait for next VM jump before serving softirq.

Finally, the patch has the usual problem that it's big, monolithic, and has unrelated changes (whitespace patching, extra lines, etc). I'd say it needs to be split into independent commits, and each part reviewed separately. For example, I'm not sure that patching each and every irq setting function in such pretty heavy manner is unavoidable, but first that part of changes would need to be separated from actual handling on softirq's in VM.

Ah, finally # 2: I believe @dpgeorge put together some softirq code during work on esp8266 port, though in the end, it wasn't needed, as hard irq worked after all.

@HenrikSolver

This comment has been minimized.

Copy link
Contributor Author

commented Sep 19, 2016

Regarding latency: softirqs should not be used if you have hard latency requirements.
Regarding # 2: Let's see what @dpgeorge has to say about it then.

@dpgeorge

This comment has been minimized.

Copy link
Member

commented Sep 20, 2016

I think I lost the stuff I did for esp8266 on softirqs, and it wasn't as well developed as what is done here.

@@ -16,6 +16,7 @@
#define MICROPY_HW_ENABLE_SERVO (0)
#define MICROPY_HW_ENABLE_DAC (1)
#define MICROPY_HW_ENABLE_CAN (1)
#define MICROPY_PY_SOFTIRQ (1)

This comment has been minimized.

Copy link
@dpgeorge

dpgeorge Sep 20, 2016

Member

You don't need these in all the board configs. Just put it in stmhal/mpconfigport.h and it'll be enabled for all boards.

This comment has been minimized.

Copy link
@HenrikSolver

HenrikSolver Sep 20, 2016

Author Contributor

OK. I will fix that.

@dpgeorge

This comment has been minimized.

Copy link
Member

commented Sep 20, 2016

It would be good to get softirq working at the REPL. I think a hook in stmhal/mphalport.c:mp_hal_stdin_rx_chr is what's needed.

@pfalcon

This comment has been minimized.

Copy link
Contributor

commented Sep 20, 2016

@dpgeorge : +1. As I write above, it would be nice to start with collecting requirements, e.g. how it should work, what are latency constraints and what can be done about them, etc. Then check implementations against them.

All in all, SoftIRQ are long-discussed topic and everyone agrees they're needed, and even various prototype implementations were tried (I consider this patch a prototype). I'd say, given that this patch already spent enough time in queue, it deserves more attention than some newly posted patches. (To follow fairness principle.)

@dpgeorge

This comment has been minimized.

Copy link
Member

commented Sep 20, 2016

Ok, yes, I agree, let's try to land this soon and consider it a prototype with API/behaviour subject to change. Would be good to eventually adapt it to esp8266 to make sure it's more general than stmhal.

@pfalcon

This comment has been minimized.

Copy link
Contributor

commented Sep 20, 2016

Well, I mean to at least provide good feedback. I gave mine above, which is expectedly conservative - I think patch needs rework, splitting into separate changes, and likely downsizing in scope, like land in mainline softirq support just for one object type for starters, Pin being the obvious choice.

I'm in particular not sure about Python-side API to set softirq handler. Just passing softirq=True to .irq() seems like good from upside, but code changes on C level seem to be big. Again, a big flat patch like this makes it hard to suggest an improvement. I'd think that support routine(s) can be factored out to handle differences of registering a hard vs soft irq handler across objects.

And well, to remind, I'm not even sure we have Python API for HardIRQ done fully right so far: #2297 ;-)

@dpgeorge

This comment has been minimized.

Copy link
Member

commented Sep 20, 2016

I'm in particular not sure about Python-side API to set softirq handler. Just passing softirq=True to .irq() seems like good from upside,

Yes the API need further thinking/discussion. For example, I think soft irq should be the default, and hard irq should be the option. This way new users don't get bitten by difficult to understand MemoryErrors in irqs, and they can move to hard irqs once they learn that they need them.

@pfalcon

This comment has been minimized.

Copy link
Contributor

commented Sep 20, 2016

For example, I think soft irq should be the default, and hard irq should be the option.

Agree, though that requires fully developed and used by all ports SoftIRQ framework.

@dhylands

This comment has been minimized.

Copy link
Contributor

commented Sep 20, 2016

A given soft-irq, once allocated should only ever be able to be on the queue once. If the soft-irq allocated object includes the link pointers (i.e. next and perhaps prev), and whatever args are needed, then your memory usage goes up proportionately with the number of soft-irqs that you allocate. And the queue is then really just a head and tail pointer.

@HenrikSolver HenrikSolver force-pushed the HenrikSolver:softirq2 branch from 2aa2647 to 0f8fcba Sep 21, 2016
        This adds the necessary base functionality
        and support in CAN, extint and usrsw.
@HenrikSolver HenrikSolver force-pushed the HenrikSolver:softirq2 branch from 066e599 to e97faf8 Oct 1, 2016
@dpgeorge

This comment has been minimized.

Copy link
Member

commented Oct 4, 2016

@HenrikSolver are you still updating this PR, or should I take it from here?

@HenrikSolver

This comment has been minimized.

Copy link
Contributor Author

commented Oct 4, 2016

Since a month back I have a new daytime job which is very interesting, which means that I have limited time to spend on Micropython things. So please go ahead and finalize it, It has been laying around long enough.

@pfalcon

This comment has been minimized.

Copy link
Contributor

commented Nov 7, 2016

@HenrikSolver, please don't close this issue, it's very important. Actually, if @dpgeorge wants to do something about HW API (per http://forum.micropython.org/viewtopic.php?f=3&t=2545, http://forum.micropython.org/viewtopic.php?f=3&t=2546), that would be among the highest priority things to work on.

@pfalcon pfalcon reopened this Nov 7, 2016
@pfalcon pfalcon added the prio-high label Nov 7, 2016
@dpgeorge

This comment has been minimized.

Copy link
Member

commented Nov 8, 2016

Once the Pin docs are finalised #2546, and 1.8.6 is released #2592 I intend to work on the machine.Pin class for all official ports to make it conform to the spec/docs. This will include having irq support, with soft irq at least and hard irq optionally.

@pfalcon

This comment has been minimized.

Copy link
Contributor

commented Nov 8, 2016

Once the Pin docs are finalised #2546, and 1.8.6 is released #2592 I intend to work on the machine.Pin class for all official ports to make it conform to the spec/docs.

The first port which needs that is as usual unix. What about it?

This will include having irq support, with soft irq at least and hard irq optionally.

So, what I thought about soft irq implementation general (and that's prompted by a certain language where event loop is builtin) is that implementations for softirq and e.g. uasyncio would in good share duplicate each other. They both involve an underlying queue to store events until they can be processed. I don't know how much more can be got from that revelation: softirq are certainly needed, and can't depend on external, optional concept like uasyncio. uasyncio be useful for low-heap systems is so far wishful thinking and should be subject of research, while softirqs should be a standard feature (configurable, but any port should be able to get them). Also, softirq queue is going to be small and simple (no priority queuing), so probably this theoretical issue at all. But such duplication arises at other places too. E.g. the author of pending esp8266 poll patch said he'd consider extending poll support to other types of objects (Pins), this effectively approached softirq functionality from another side and also would duplicate functionality which would be expected to be handled by uasyncio.

@dpgeorge

This comment has been minimized.

Copy link
Member

commented Mar 20, 2017

This has been superseded by the merge of #2878.

@dpgeorge dpgeorge closed this Mar 20, 2017
tannewt added a commit to tannewt/circuitpython that referenced this pull request Aug 22, 2019
typo in IncrementalEncoder.c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.