Add message queue to prevent race-conditions when sending replies#21
Add message queue to prevent race-conditions when sending replies#21bruno-f-cruz wants to merge 2 commits intomainfrom
Conversation
| uint64_t timestamp; | ||
| uint16_t analog_data[3]; | ||
| }; | ||
| struct harp_event_t |
There was a problem hiding this comment.
I recommend using msg_t in harp_message.h or at least use msg_header_t, which is everything up to the payload.
This message struct also doesn't explicitly indicate that the msg_type is an EVENT, so I might suggest renaming it to enqueue_harp_msg instead.
| {(uint8_t*)&app_regs.analog_data, sizeof(app_regs.analog_data), U16} | ||
| }; | ||
|
|
||
| inline void enqueue_harp_event(msg_type_t msg_type, uint8_t reg_address, uint64_t timestamp) |
There was a problem hiding this comment.
From the way this is constructed, it's essentially making a copy of the current register contents so that they can be managed in a queue. That's assuming the only time we call this function is after the contents of the register are up-to-date. That's totaly fine; it's probably worth mentioning somewhere though.
| // We arbitrarily choose to share the timestamp for the two events | ||
| uint64_t harp_time_us = HarpCore::harp_time_us_64(); | ||
| HarpCore::send_harp_reply(EVENT, APP_REG_START_ADDRESS + 2, harp_time_us); | ||
| enqueue_harp_event(EVENT, (uint8_t)(APP_REG_START_ADDRESS + 2), harp_time_us); |
There was a problem hiding this comment.
Suggestion: I usually make aliases for (uint8_t)(APP_REG_START_ADDRESS + 2), so it's a little clearer in the code what's happening, and you can manage address offsets in one place if you need to insert registers later rather than having to search through the code for these.
| event.msg_type = msg_type; | ||
| event.num_bytes = spec.num_bytes; | ||
| event.payload_type = spec.payload_type; | ||
| memcpy(event.payload, (const void*)spec.base_ptr, spec.num_bytes); |
There was a problem hiding this comment.
This is still a race condition.
Because interrupt service routines (gpio_callback in this case) can fire at any time, if a register payload is >1byte, the cpu may be partially changing the values in the registers, and the ISR can fire and then call this memcpy which replaces everything. When the cpu resumes, the previous bytes would have been overwritten by the ISR, resulting in data corruption.
There are a few ways to avoid this.
- heavy-handed, but wrap your main cpu code that changes memory that could also be altered in the ISR with
uint32_t int_status = save_and_disable_interrupts()andrestore_interrupts(int_status). Without further reading, I can't say that interrupts that fire during this time queue themselves, so we should investigate that behavior for the rp2xxx cores. If they do not, they you may potentially miss GPIO changes. - light, but kinda hacky: if your register is, at most, a single
uint32_tin size, just use the assignment operator.memcpycopies one byte at a time, but assignment copies oneuint32_tat a time. - Create a full harp message in the ISR but only apply it to the register state in the main loop during the update function where you also do the sending. This is, by far the easiest, but assumes you aren't relying on the freshest register data being present for that register elsewhere.
There was a problem hiding this comment.
Generally ISRs should be as short as possible. I usually make custom, teeny structs to only represent the data that needs to get changed (timestamp and local variables), queue that small chunk of data, and then apply the full change to the register in the update loop.
There was a problem hiding this comment.
The first design was actually using 3. I guess you think this strategy is unnecessarily complicated especially given the features of this board? Do you have a sense of the sort of delays expected between queueing the message and grabbing the reg state? I was just afraid of triggering on HIGH but sending LOW if the pulse was super short.
| } | ||
|
|
||
| harp_event_t harp_event; | ||
| while (events_active && queue_try_remove(&harp_event_queue, &harp_event)) |
There was a problem hiding this comment.
At minimum, you should probably drain the queue regardless of whether or not you are muted; otherwise queue_try_add will lock everything up. (This is assuming you're ok with losing all events when things are muted, which must be the case by nature of being muted.)
2 participants
I still need to test this with a real device
Closes #18
I am not exactly sure which event was triggering the issue, but I think this is probably a generally safer solution for the race conditions. Here we simply add a the struct with the register and the timestamp to a queue that is dequeued on the main loop.
We attach the dequeue'ing to the
update_app_statefunction that, if i understand correctly, runs on every iteration of the main loop:https://github.com/harp-tech/core.pico/blob/20103422f33f4cf4315a8aa880e25d0829dc8d06/firmware/src/harp_core.cpp#L57
For now I left the adc queue untouched since:
queue_try_addcall.If we want to clean up the code we could consider merging