Interrupt defines #1565
Interrupt defines #1565
Conversation
|
It seems I might have left some clutter in the commit history. I can fix that if necessary. |
Yes, please consider using rebase-based devel workflow to avoid that automagically: https://github.com/micropython/micropython/wiki/DevelWorkflow |
|
Yes, I know about the work flow, I simply forgot this time after resolving merge conflicts -- it seems I am in a race with someone merging tweaks to can.c so I need to refresh anyway. |
|
irq.h might also be a resonable place to put these (since that's where the enabled_irq/disable_irq functions go. If I did that, I'd probably use IRQ_ rather then INTR_ as a prefix. Pro Tip: If I need to do those types of renames, I then create a patch file, and do a search/replace on the patch file. I use git format-patch to create the patch file. |
|
irq.h also seems a reasonable place. I'll re-submit after we get a decision: |
|
In irq.h would be my preference.
|
|
OK, irq.h it will be. Also, as @dhylands points out, IRQ_ is more consistent with ARM naming conventions, so that seems worth changing. |
18b2c64 to
1487cbe
Compare
|
Should be ready for review/merge. |
|
@dhylands
Not at issue in the pull request. This pull request intentionally has zero functionality impact. It is a maintainability improvement. I think you should open a new issue(s) for interrupt priority adjustments. The point of this patch is to contain those adjustments to a single, obvious, file. |
Well the #1555 thread actually has nothing to do with FLASH. It was UART/SD and interrupt priority wasn't a factor (the issue seems to be about interrupts being disabled). So - my comment wasn't really about your PR, but more about the comment relating to FLASH. I can't imagine trying to do I/O from an ISR? Or maybe this is referring the relationship between the storage timer (which triggers periodic flushes) and the FLASH IRQ? |
|
OK, there is something missing here. You only said:
And somehow from that we are to understand you are talking about a code comment relating to FLASH? That could have been better sign-posted. I will assume you are talking about: // Flash IRQ must be higher priority than interrupts of all those components
// that rely on the flash storage.
#define IRQ_PRI_FLASH 0x1
#define IRQ_SUBPRI_FLASH 0x1which if you go and review the source code being changed, is simply copying the comment from where the irq priority was previously being set, to the new file. Leaving it only in the file where the #define is used is inviting trouble. If it was important to document that restriction yesterday, it is important to document the restriction today, and to have it documented in the place where someone is looking at it to change it. As to the deeper meaning of why the flash irq needs to be at such a high priority, I have nothing to say. Again, the point here was to make a functionality-neutral improvement in maintainability. |
|
Yep - I assumed that because it was a line comment with a position, it was clear that I was talking about the FLASH_IRQ. And like the RTC comment, I wouldn't consider this a reason not to go ahead with the PR. It was just something I noticed while looking at the PR, and as you suggested with the RTC one, any actual changes should probably go in a separate PR. This is just discussion. To be clear- I'm all in favor of your PR. |
|
@dhylands OK, understood. I am curious as to why the flash irq needs to be such a high priority. I was planning to dig into that a little bit in the context of getting the UART to work at faster than a slow jog. |
|
With the priority mechanism that we use (PRIORITYGROUP_4) subpriorities have no meaning, so they should all be 0. But, that's something we can fix later. And, it's nice to have SUBPRI #defines so we can change them in one place if one ever uses a different grouping scheme. |
|
Merged in 32b3549. |
As noted in issue #1555 , for the stmhal currently the interrupt priorities are hard-coded and spread across several source files. This patch is purely a code clean-up text substitution, creating #defines for interrupt levels in the new file stm_hal/interrupt_priorities.h.
It might be debatable whether or not a small hand full of defines need their own file. mphalport.h might be a reasonable place for them, reducing file clutter in the main development directory. Some also might object to the length of the file name.