Rework lock_core / timers #378

Merged
kilograham merged 1 commit from lock_core_rework into develop 2021-05-05 11:46:25 -05:00
kilograham commented 2021-05-04 14:18:54 -05:00 (Migrated from github.com)
  • Add recursive_mutex
  • Make all locking primitives and sleep use common overridable wait/notify support to allow RTOS
    implementations to replace WFE/SEV with something more appropriate
  • Add busy_wait_ms
- Add recursive_mutex - Make all locking primitives and sleep use common overridable wait/notify support to allow RTOS implementations to replace WFE/SEV with something more appropriate - Add busy_wait_ms
Wren6991 (Migrated from github.com) reviewed 2021-05-04 14:18:54 -05:00
liamfraser (Migrated from github.com) approved these changes 2021-05-05 06:00:48 -05:00
liamfraser (Migrated from github.com) left a comment

Looks okay to me. I spent a while reviewing as requested by Graham.

Looks okay to me. I spent a while reviewing as requested by Graham.
lurch (Migrated from github.com) reviewed 2021-05-06 04:49:44 -05:00
@ -13,0 +20,4 @@
* lock which is used only to protect the contents of the rest of the structure as part of implementing the synchronization
* primitive. As such, the spin_lock member of lock core is never still held on return from any function for the primitive.
*
* \ref critical_section is an exceptional case in that it does not have a lock_core_t and simply wraps a pin lock, providing
lurch (Migrated from github.com) commented 2021-05-06 04:49:44 -05:00

"wraps a pin lock" -> "wraps a spin lock" ?

"wraps a pin lock" -> "wraps a spin lock" ?
lurch (Migrated from github.com) reviewed 2021-05-06 04:51:08 -05:00
@ -13,0 +28,4 @@
*
* By default the SDK just uses the processors' events via SEV and WEV for notification and blocking as these are sufficient for
* cross core, and notification from interrupt handlers. However macros are defined in this file that abstract the wait
* and notify mechanisms to allow the SDK locking functions to effectively be used within an RTOS on other environment.
lurch (Migrated from github.com) commented 2021-05-06 04:51:08 -05:00

"RTOS on other environment" -> "RTOS or other environment" ?

"RTOS on other environment" -> "RTOS or other environment" ?
lurch (Migrated from github.com) reviewed 2021-05-06 04:52:39 -05:00
@ -13,0 +32,4 @@
*
* When implementing an RTOS, it is desirable for the SDK synchronization primitives that wait, to block the calling task (and immediately yield),
* and those that notify, to wake a blocked task which isn't on processor. At least the wait macro implementation needs to be atomic with the protecting
* spin_lock unlock from the callers point of view; i.e. the task should unlock the spin lock when as it starts its wait. Such implementation is
lurch (Migrated from github.com) commented 2021-05-06 04:52:38 -05:00

"spin lock when as it starts" -> "spin lock when it starts" ?

"spin lock when as it starts" -> "spin lock when it starts" ?
lurch (Migrated from github.com) reviewed 2021-05-06 04:57:59 -05:00
@ -34,0 +114,4 @@
*
* \param lock the lock_core for the primitive which needs to block
* \param save the uint32_t value that should be passed to spin_unlock when the spin lock is unlocked. (i.e. the `PRIMASK`
* state when the spin lock was acquire
lurch (Migrated from github.com) commented 2021-05-06 04:57:59 -05:00

Mixture of using "this method" and "this macro" (although I dunno if this difference is significant?)

Mixture of using "this method" and "this macro" (although I dunno if this difference is significant?)
lurch (Migrated from github.com) reviewed 2021-05-06 05:03:28 -05:00
@ -34,0 +182,4 @@
* RTOS which is able to block the current task until the scheduler tick before
* the given time)
*
* \param until the \ref absolute_time_t value
lurch (Migrated from github.com) commented 2021-05-06 05:03:28 -05:00

"if can" -> "it can"

"if can" -> "it can"
lurch (Migrated from github.com) reviewed 2021-05-06 05:09:53 -05:00
@ -36,2 +39,4 @@
#define MAX_RECURSION_STATE ((uint8_t)255)
/*! \brief Initialise a mutex structure
lurch (Migrated from github.com) commented 2021-05-06 05:09:53 -05:00

Hopefully people won't accidentally assume that MAX_RECURSION_STATE corresponds to the "1 is a maxed out recursive lock" described above?

Hopefully people won't accidentally assume that `MAX_RECURSION_STATE` corresponds to the "1 is a maxed out recursive lock" described above?
lurch (Migrated from github.com) reviewed 2021-05-06 05:13:01 -05:00
@ -35,0 +33,4 @@
lock_owner_id_t owner; //! owner id LOCK_INVALID_OWNER_ID for unowned
uint8_t recursion_state; //! 0 means non recursive (owner or unowned)
//! 1 is a maxed out recursive lock
//! 2-254 is an owned lock
lurch (Migrated from github.com) commented 2021-05-06 05:13:01 -05:00

I wonder if it'd have been worth making this uint8_t a typedef (e.g. recursion_state_t) ? 🤷 Or perhaps we already have enough typedefs scattered throughout the place.

I wonder if it'd have been worth making this `uint8_t` a typedef (e.g. `recursion_state_t`) ? :shrug: Or perhaps we already have enough typedefs scattered throughout the place.
Sign in to join this conversation.
No milestone
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
gh-raspberrypi/pico-sdk!378
No description provided.