Solution for mutex problem in CAN read ISR #14688
Conversation
interrupt is enabled.
ciarmcom
added
the
release-type: patch
|
@MubeenHCLite, thank you for your changes. |
targets/TARGET_STM/can_api.c
Outdated
| static uint32_t can_irq_ids[CAN_NUM] = {0}; | ||
| static can_irq_handler irq_handler; | ||
| static uint32_t rx_irq_status = DISABLED; |
There was a problem hiding this comment.
Does this not need to be either an array [CAN_NUM] or a flag inside the can_t object? Probably the former, but I'm not quite sure how this is handling multiple instances.
targets/TARGET_STM/can_api.c
Outdated
| @@ -646,8 +646,12 @@ void can_irq_set(can_t *obj, CanIrqType type, uint32_t enable) | |||
| #include <string.h> | |||
| #include <inttypes.h> | |||
|
|
|||
| #define ENABLED true | |||
There was a problem hiding this comment.
Rather than inventing your own values, just name the variable so booleans work:
static bool rx_irq_enabled = false;
| @@ -998,6 +1003,10 @@ int can_read(can_t *obj, CAN_Message *msg, int handle) | |||
| can->RF1R |= CAN_RF1R_RFOM1; | |||
| } | |||
|
|
|||
| if(rx_irq_status == ENABLED) { | |||
| __HAL_CAN_ENABLE_IT(&obj->CanHandle, CAN_IT_FMP0); | |||
There was a problem hiding this comment.
Do we believe this is race clear? If new data has already arrived before this line, will this generate an interrupt?
Hopefully this is solid, but it's possible you may have to do
enter critical section
enable FMP0 interrupt
release FIFO
disable critical section
to make sure the thing is ready to generate interrupts as the FIFO is released. I don't know if it's worth doing that just in case.
|
Disabling IRQ, your serious ? You brings it to a thread that has a latency of at least 1ms and give it the mission to treat frames that can be sent at 1Mbit/s with between 45 et 129 bits per frame (so 45 to 129µs per frame) and a fifo of only 3 frames inside the chips... This will never works if bus is loaded at more than 10%. That is not a good idea ! |
|
@haarkon Counter proposal for this pull request? |
|
Not sure to be good enough to be able to create it, but the solution is : |
"At least"? It shouldn't be anywhere near that bad. If it's a high priority thread, can wake a thread with only tens of microseconds of latency. IRQ latency due to critical sections still dominates in practice (in Mbed OS), not the thread switching/waking time. People tend to disable IRQs globally for many tens of microseconds or worse. But yes, you would want it to be a high priority thread. This is the technique used in various SPI and Ethernet drivers. For example, we've got a number of IEEE 802.15.4 radio drivers that have to be able to do a TX->RX turnaround within 192us reliably to catch an ack, and they do this from high-priority thread context without issue. A normal priority thread might end up having to wait for round-robin scheduling, which is maybe the scenario you're thinking of for "1ms". It may be that it still isn't possible to achieve performance from thread reliably, but you need to recheck the numbers on your estimates. If that is the case, then this RX API becomes like A |
|
Ok maybe less than 1ms if I increase the thread priority, but:
For me all MUTEX should be removed and user have to add them or not. End of the story! |
|
Your IRQ handler should indeed be waking your thread from a wait state using a semaphore or event flag. In this patch I believe the IRQ is only disabled between IRQ signalling and the thread doing the read. I agree that needing to make a high-priority "data-pump" thread is awkward, and apps shouldn't have to worry about that level of complexity. Just as they shouldn't have to worry about needing interrupt handlers at all - blocking until data arrives is easier. Which is why the You've always been able to attempt direct ISR HAL access if you want by simply overriding the mutex - all(?) of the HAL objects have their Radio drivers wanting to do SPI direct from IRQ used to have an The "mutex by default" on the objects is a first line of defense against multithreaded programs performing re-entrant access on the single-threaded HAL. For some people it's too much, and in some cases it's not enough. If there are cases where an API is only viable from IRQ, but still has the mutex fitted, then that is indeed paradoxical. It sounds like you would rather have |
Yes, it could be a good idea to activate a flag in the ISR (to bring a thread from Wait to Ready) and disabling the IRQ then to be able to exit the ISR... But it's not what has been proposed here. Just to disable the IRQ... I haven't seen any flag raised, neither the creation of an ISR that does all the things you talk about, that are to me, a good point of view. But, if your proposal may work, it's completly crazy to do such things when the problem is comming from the almost useless mutex... So yes a RawCAN / BufferedCAN should be better than this solution whitch is swipping dust under the carpet (IRQ causes problem? Then action is: remove the IRQ)... Yes, I'm for the removal of most of the Mutex, I beleive that if you decide to play with Mbed you need to deal with your own Mutex (and add them or not, depending of your code : do you share this ressource? Does it interfere with an ISR? etc.), not to deal with mutex that has been widely put everywhere, even if not needed, just "in case of". |
|
@ARMmbed/team-st-mcd |
Yes, this is just a change to the HAL, temporarily disabling the IRQ before calling the user IRQ handler. The originator of the patch is creating it to make their own thread-waking IRQ handler work, I believe. I think you've been somewhat misinterpreting the effect of the patch. This shouldn't stop anyone's existing IRQ handlers from working, I hope. (People already using unlocked objects or driving the HAL API otherwise...)
I'm somewhat sympathetic to this view. The HAL object mutexes were strapped in very early in Mbed OS 5, and there's not been a lot of follow-up or review. However things like "do you share this resource" requires system-level knowledge - when assembling libraries and subsystem drivers, they won't know that. The existing system where most things get the default mutex and no-one has to worry about conflicts, but 1 time-critical driver has a dedicated bus and unlocked HAL object has worked out well in practice. But API consistency and backwards compatibility is a thing, so just stripping mutexes from 1 class in isolation isn't really an option. Whereas adding |
|
This pull request has automatically been marked as stale because it has had no recent activity. @MubeenHCLite, please carry out any necessary work to get the changes merged. Thank you for your contributions. |
|
Besides there is fundamental issue with CAN (tracked in the issue tracker). @MubeenHCLite Can you respond t o reviewers comment (from @kjbracey-arm to the changes in this pull request) ? |
|
The implementation of suggested changes are in progress, will be pushed soon |
|
CI started |
Jenkins CI Test : ❌ FAILEDBuild Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & ArtifactsCLICK for Detailed Summary
|
|
There are errors in building related to RawCAN |
|
CI restarted |
Jenkins CI Test : ✔️ SUCCESSBuild Number: 2 | 🔒 Jenkins CI Job | 🌐 Logs & ArtifactsCLICK for Detailed Summary
|
|
As @kjbracey-arm also approved previously, only one remaining action to take: documentation update for this new RawCAN: https://github.com/ARMmbed/mbed-os-5-docs/tree/development/docs/api/drivers - new .md file should be created. @MubeenHCLite Can you please |
The documentation update mentioned here is in a different repository. Cannot be updated as a part of this PR This can be merged. |
|
I've just noticed the merge commit in the history, please rebase the next time. |
Summary of changes
Implementation for can_read to be deferred to thread context when rx interrupt is enabled.
As can_read is protected by mutex(mutex is not allowed in ISRs in Mbed) and read is only possible way to clear interrupts in bxCAN in STM controllers, reads are to be deferred to thread context.
In addition RawCAN (unlocked apis) is added, which can be used in case if CAN object is accessed by a single thread.
Impact of changes
Migration actions required
Documentation
Pull request type
Test results
Reviewers