sys: Removed irq_handler

[maribu]

Contribution description

This PR drops support for irq_handler, which allowed to dispatch work to a dedicated worker thread to avoid the interrupt context problem.

Reasoning

The API for dispatching work to one or more dedicated worker threads is not yet ready to be exposed to a broader public in a release, as the API is currently under heavy discussion. Likely, the feature will be exposed using the event API in the coming days. In order to avoid having to maintain two APIs for the same functionality, this should be removed before the 2019.10 release is officially released.

Testing procedure

The CI should do the heavy lifting here.

Issues/PRs references

Discussion of the API in #12459, #12474 and #12480

Revert of #10555

[maribu]

As this is a bigger change during soft hard feature freeze, two ACK should be given before merge.

[gschorcht]

We have an agreement to remove it.

[jia200x]

This PR should go to master and then backported to the 2019.10 branch.

[maribu]

This PR should go to master and then backported to the 2019.10 branch.

I 100% agree that this is the correct process for any new feature that we still want to get into the release or any bugfix. But I'm not sure if it is the right thing to do here. Everyone involved agrees that the feature of irq_handler should be integrated into RIOT, only the how is being discussed about. I think that at least parts of irq_handler are likely reused - even if it will only be the documentation or the test. So removing it from master will make that reuse (in the master branch) harder.

The reason of this PR is this: I really think we should ensure to not have to deprecate a feature just because it "slipped" into a release a few days before a huge API change. That would only frustrate users and maintainers alike. Users don't want to see an API that they just adopted to be deprecated directly after, and maintainers don't want to maintain an depricated API if that could have been easily avoided.

[MrKevinWeiss]

For some reason I thought we should deprecate for two release cycles before removing? I don't know too much about this module but it also seems strange to not first change master then backport.

[benpicco]

#10555 was only merged in August, so it hasn't been part of a release yet.

[gschorcht]

irq_handler was merged on Aug, 13rd. That is, it was not part of release 2019.07. If we would backport its removal to 2019.10, it was officially never in any release.

[MrKevinWeiss]

OK, I don't think depreciation is needed then however I still would like to see master updated first.

[maribu]

I still would like to see master updated first

Please read my post above.

[MrKevinWeiss]

Ahh looking at this it is just a revert. I guess reverting something in the release branch is OK... Maybe add something in the commit message about a revert of #10555 then I am OK. I was just a bit worried about things diverging.

[maribu]

@MrKevinWeiss: I amended the commit message as suggested. I also added a note to why it was not deprecated as usual in the commit message

[maribu]

OK. After fixing some typos I think the commit message is now fine

Comments addressed

[MrKevinWeiss]

Can we hold off for a small while. Some other people have some minor concerns that I am hoping to address shortly. @RIOT-OS/maintainers thoughts?

[kb2ma]

As this is a bigger change during soft feature freeze, two ACK should be given before merge.

We are in hard freeze, not soft freeze. There is nothing in the release guidelines that says we need two ACKs to remove a feature during hard freeze [1]. The only guideline I see for changes during hard freeze is that only bug fixes are acceptable during hard freeze:

When hard feature freeze emerges there should be at most bugfix pull requests open with a label for this release.

For a change to the release process, it is essential to bring in the release manager into the discussion. I just happened to hear about this in an oblique way. I am blocking this PR until I understand better why we are making this change.

[1] https://github.com/RIOT-OS/RIOT/wiki/%5Bdraft%5D-Managing-a-Release

[kb2ma]

I have not had a chance to look closely at this issue yet, and honestly I need to get out the door to go to work this morning ;-). I am concerned that we have put a lot of work into RC1 testing at this point, and I don't want to disturb that work.

Another alternative is to mark up the release notes about this feature. We can say that this irq_handler as very experimental and likely to be replaced in the very near future. So, if someone wants to use it, it is totally at their own risk. In fact, if someone uses it in its current state, maybe that would even help with the re-implementation in the event API.

[miri64]

While it is technically a feature change, at least in my opinion, there is no real harm in removing a feature from a release altogether. It isn't really interfering with release testing as to my understanding the feature isn't even used yet by our code base, so it would be like merging a feature after feature freeze. Also from the perspective of bug fixing such a change as this would make sense: Say a new feature contains a lot of bugs by design but some of its code base is somehow salvageable (not the case here, I know). I rather would than see this feature removed from the release candidate, than putting a lot of effort in trying to fix the underlying issues (potentially) huge backporting fixes.

On the issue of the release manager not being informed what is happening:

1. Could some of the people involved with this feature (@maribu, @gschorcht, @jia200x, @haukepetersen, @kaspar030) give a quick run-down for @kb2ma what led to this decision?
2. For future reference, such a decision should be announced to maintainers in the future, so people are aware of it.

[maribu]

1. Could some of the people involved with this feature (@maribu, @gschorcht, @jia200x, @haukepetersen, @kaspar030) give a quick run-down for @kb2ma what led to this decision?

So far the PR is still under review, so decisions are yet to be made. (Except for the decision on opening a PR, which was my decision. I refer to the PR description or the commit message for the reasoning.)

2. For future reference, such a decision should be announced to maintainers in the future, so people are aware of it.

I doubt that other people will dig through the conversations of old PRs to stumble by chance over this sentence, or that you can expect them to do so. I created a PR to add this info to MAINTAINING.md in #12500

[kb2ma]

Thanks to @maribu for raising the flag to remove/disable a feature that likely will be superceded in the near future. I have read through the PRs that added irq_handler as well as tasklets and event handlers. I agree that there is a lot of risk for use of irq_handler from #10555 in release 2019.10.

I understand that there is rough concensus on the new event handling mechanism, but there is significant work to hammer out an agreeable implementation. Certainly there is nothing mature enough to replace the existing irq_handler. Also, this whole discussion has occurred within the last ten days, since hard feature freeze for the release.

However, I think the remedy in this PR is too disruptive to our development and release process. My main objection is the removal of irq_handler only from the 2019.10 release branch. This means a discontinuity/branch/fork with the code in master that we need to keep in mind -- "note to self: remove irq_handler." It also opens the door to this kind of discontinuity in the future for other purposes.

We need to find a manageable way to integrate this kind of last minute change of mind with our rapidly developing code base, rather than attempt to "stop the world" for the release branch. So, if the main concern is the length of time to deprecate a new feature that's gone into master, let's make a special case for code that needs to be deprecated just as we're going into release.

I propose that we add a bold note at the top of irq_handler.h that says the module is deprecated and scheduled to be removed with release 2020.01. This addition will follow the usual hard freeze process of addition to master and then backport to the 2019.10 branch. We also can call out this unusual change in the release notes. If there is some agreement, I'll create a PR.

In this way, users of the release will see not to use irq_handler, and any one looking at master also will see it is on its way out. Then we can remove the code when we're ready in the next few months. And if for some crazy reason we decide to keep it, we only need to remove the deprecation notice.

[miri64]

However, I think the remedy in this PR is too disruptive to our development and release process. My main objection is the removal of irq_handler only from the 2019.10 release branch. This means a discontinuity/branch/fork with the code in master that we need to keep in mind -- "note to self: remove irq_handler." It also opens the door to this kind of discontinuity in the future for other purposes.

I don't get this argument. As far as I understand, development involving API changes will still happen in master to this module. So reverting it, only to re-add it later in master seems impractical. On the other hand, reverting its initial import just in the release branch results in a similar situation of it being merged after feature freeze. master and 2019.10-branch are already diverging, removing something from the 2019.10-branch doesn't increase the discontinuity of that branch as merging a new feature to or reworking an API in master would.

[maribu]

@kb2ma: Marking the feature as deprecated from the beginning and scheduled for removal in the next release is a good idea to tackle the maintenance issue.

But I bet it isn't a benefit for users to get a feature shipped with a "do not even think about using me" label sticking on it. If they read that label, they will wonder why it has been shipped at all and why they had to waste some of their lifetime to read the doc of a dead API. If they don't read it and just start to use it, they will waste even more of their lifetime. So for users it would be better to not get the API at all.

I honesty don't understand the point with the discontinuity between releases and the master branch. Technically they move apart with every PR merged in master after tge release branch has been created that doesn't get backported - and new features are explicitly not backported. So the discontinuity seems to be by design and fully intentional, doesn't it?

I do understand that this type of PR is not handled by the rules for the release process. But we could likely end up in the same situation in the future. E.g. I hope that ztimer gets into master soon, but I coule imagine that it will not reach the maturity for being included in a stable release in time for the next release. That would be pretty much the same situation as we have now.

So to me it is a trade of between what is best for the users vs. not being to disruptive for the release process. I personally am fine with both options (removing the API or flagging it as deprecated and to be removed in the next release).

I don't understand the plans for this module well enough to stand in the way. I also am trying to keep up with testing and backports. I leave it to those more familiar with the issues here to decide on the best approach. I am happy at least that the existing process has been considered and hopefully will be improved.

[maribu]

@RIOT-OS/maintainers: Please decide on this. I'm happy to open an alternative PR that marks sys/irq_handler as deprecated and scheduled for removal in the next release, if this is prevent.

I'm pretty sure that just shipping it (for the first time) without any warning while it is about to be heavily refactored in master is going to haunt as for a while. So in my opinion we really should decide on what to do about that, before the release is made public.

[kb2ma]

Thanks @maribu, for posting the reminder. Yes, I created RC2 for 2019.10 yesterday, with the hope/expectation that we won't find any other issues. I will go ahead and create that alternative PR with the deprecation since I suggested it, to make it easy for anyone to comment.

I am happy to create an RC3 for one of these PRs, but it will need to happen soon.

[kaspar030]

So to me it is a trade of between what is best for the users vs. not being to disruptive for the release process. I personally am fine with both options (removing the API or flagging it as deprecated and to be removed in the next release).

This. @kb2ma should decide whether he can merge the removal to the release branch. A note in the release notes should be fine if not.

Let's not waste too much time worrying about our three users and use it to make the next two happy!

[kaspar030]

I will go ahead and create that alternative PR with the deprecation since I suggested it, to make it easy for anyone to comment.

I think a simple release note entry is fine ("irq_handler is new but will be removed in the next release"). For the next release notes, we can point to instructions how to use the successor. No need for a new RC.

[kb2ma]

@kb2ma should decide whether he can merge the removal to the release branch.

I do not think the content of this PR and the timing of it justifies the change to the release process at this time, this close to the release. I agree it makes sense to add to the release notes as you suggest.

[maribu]

Closed in favor of an addition to the release notes.