Rework lock_core / timers #378
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- 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
liamfraser
approved these changes
May 5, 2021
Contributor
liamfraser
left a comment
liamfraser
left a comment
There was a problem hiding this comment.
Looks okay to me. I spent a while reviewing as requested by Graham.
lurch
reviewed
May 6, 2021
| * 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 |
Contributor
There was a problem hiding this comment.
"wraps a pin lock" -> "wraps a spin lock" ?
lurch
reviewed
May 6, 2021
| * | ||
| * 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. |
Contributor
There was a problem hiding this comment.
"RTOS on other environment" -> "RTOS or other environment" ?
lurch
reviewed
May 6, 2021
| * | ||
| * 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 |
Contributor
There was a problem hiding this comment.
"spin lock when as it starts" -> "spin lock when it starts" ?
lurch
reviewed
May 6, 2021
Comment on lines
+107
to
+113
| * In an ideal implementation, this method would return exactly after the corresponding lock_internal_spin_unlock_with_notify | ||
| * has subsequently been called on the same lock instance, however this method is free to return at _any_ point before that; | ||
| * this macro is _always_ used in a loop which locks the spin lock, checks the internal locking primitive state and then | ||
| * waits again if the calling thread should not proceed. | ||
| * | ||
| * By default this macro simply unlocks the spin lock, and then performs a WFE, but may be overridden | ||
| * (e.g. to actually block the RTOS task). |
Contributor
There was a problem hiding this comment.
Mixture of using "this method" and "this macro" (although I dunno if this difference is significant?)
lurch
reviewed
May 6, 2021
| * This method is provided for cases where the caller has no useful work to do | ||
| * until the specified time. | ||
| * | ||
| * By default this method does nothing, however if can be overridden (for example by an |
lurch
reviewed
May 6, 2021
| //! 255 is an un-owned lock | ||
| } mutex_t; | ||
|
|
||
| #define MAX_RECURSION_STATE ((uint8_t)255) |
Contributor
There was a problem hiding this comment.
Hopefully people won't accidentally assume that MAX_RECURSION_STATE corresponds to the "1 is a maxed out recursive lock" described above?
lurch
reviewed
May 6, 2021
| lock_core_t core; | ||
| int8_t owner; //! core number or -1 for unowned | ||
| lock_owner_id_t owner; //! owner id LOCK_INVALID_OWNER_ID for unowned | ||
| uint8_t recursion_state; //! 0 means non recursive (owner or unowned) |
Contributor
There was a problem hiding this comment.
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.
kilograham
added a commit
that referenced
this pull request
Jun 3, 2021
See release notes for more descriptive details * Delete some redundant CMake parts (#240) * pio: Add 'pragma once' to generated header files (#237) * pio: allow programs with 32 instructions (#236) * Fixup incorrect doxygen for multicore_fifo_wready * Add param-validation to spin_lock_instance * Fix back-to-front description of IRQ priority in doxygen (#245) * Fix ROSC typo (#259) * Typo (#251) * Add gpio_get_out_level() accessor, and correct SIO GPIO_OUT struct ty… (#247) * Add gpio_get_out_level() accessor, and correct SIO GPIO_OUT struct type from WO to RW * Add pico_get_unique_board_id_string API (#281) * Clean up -Wconversion=error issues * move PLL reset code from clocks driver to pll driver (#110) * Don't clear PLL PWR/FBDIV after reset as unnecessary. Call out in runtime.c why USB/syscfg aren't reset. * i2c: set hold time of SDA during transmit to an appropriate value (#273) * i2c: set hold time of SDA during transmit to 2 for TCS34725 color sensor * i2c: fix issues in i2c_write_blocking_internal * i2c: rename sda_hold_count to sda_tx_hold_count * use assert rather than invalid_params_if for internal consistency checks * i2c: use a more appropriate sda tx hold time at higher baudrates * i2c: reduce 120/1e9 to the smallest possible integer numerator and denominator * Update NULL GPIO function to 0x1f (#320) * i2c: set high and low times to values that conform to the i2c specification (#314) * Make flash_do_cmd public (#269) * Fix implementation config listing in structs/i2c.h (#324) * Clarify that cache is flushed, but that function is intended for low-level metadata access during startup (#322) * Fix implementation config listing in structs/i2c.h (#325) * Fix param-validation for PIO sideset encoding (#311) * Remove MASTER_ON_HOLD bit from I2C status registers. Fix typos. (#326) * Fixing arithmetic underflow in SPI I/O loops #337 (#338) * Source code licence clarification (#340) * Updated existing Pimoroni board headers to match latest style, and added a new board (#343) * Added new pimoroni board headers * SPI Definitions for SparkFun boards (#344) * SPI Definitions for SparkFun boards * Clarify multicore_fifo doxygen (#323) Based on my observations in #284 * correct adafruit flash size for itsybitsy and qt rp2040 (#348) from 4 MB to 8 MB * Small typos (#366) * make spi_init return baud rate set (#296) * Fix path + typo in README.md (#347) * Fix path + typo in README.md * Remove incorrect path change * Remove typo * disable core 0 SIO FIFO IRQ handler during core 1 launch in case someone has already installed one (#375) * add PICO_DIVIDER_DISABLE_INTERRUPTS flag which makes PICO_DIVIDER disable interrupts around division rather than using co-operative guards to protect nested use (i.e. within IRQ/exception). Use of this flag can simplify porting of RTOSes but with a different performance profile (#372) * make all non hardware_ libraries foo add C preprocessor definition LIB_FOO=1, and remove bespoke definitions which were all undocumented anyway (#374) * Change various (confusing to user) message to be DEBUG only (#365) * add small delay to stdio_get_until to prevent starvation of USB IRQ handler due to in use mutex. build was non deterministic due to missing link wrapping of getchar (#364) * Some cmake build improvements (#376) * Change some cmake output to DEBUG level Make SDK build more consistent with other libraries (use an INTERFACE marker library for inclusion tests) Add PICO_SDK_PRE_LIST_FILES, PICO_SDK_POST_LIST_FILES build vars * fix typo * remove leftover debugging message * i2c: improve communication with i2c devices in i2c_write_blocking * Definitions for IC_TX_BUFFER_DEPTH inconsistent (fixes #335) (#381) * Add hardware_exception for setting exception handlers at runtime (#380) * add __always_inline to trivial super low level inline functions (#379) * Rework lock_core / timers (#378) * remove spurious sys/select.h include (#377) * Fixup IRQ_PRIORITY #define values (#393) * Correct doxygen for mutex_try_enter (#392) * Fix a bunch of doxygen typos (#391) * Rework ordering of cmake, so that libraries in subdirectories can add to internal lists as PICO_SDK_POST_LIST_FILES, PICO_CONFIG_HEADER_FILES etc. (#382) * Fix some hardware_library dependencies (#383) * make host pico_platform.h and binary_info.h CMakeLists.txt safe for inclusion in non SDK build (#388) * Add basic CMSIS core headers (#384) * Fix the PICO_CONFIG default value for PICO_CMSIS_RENAME_EXCEPTIONS (#399) * add timeout_us/until to mutex/sem blocking methods (#402) * Fixup divider save_restore for floating point too; improve tests (#405) * fix pico_promote_common_scope_vars (#397) * add comment about using clk_gpout0 enable bit (Fixes #413) * pioasm: prevent double inclusion for C SDK generated headers (#417) * Add missing cast to uint32_t in hw_divider_u32_quotient for host (#436) * Optional feature to get the max level that has ever been held by a queue (#444) * Fix wrong format string in alarm_pool_dump_key (#437) * Add support for Arduino Nano RP2040 Connect (#425) * Add support for Arduino Nano RP2040 Connect * Add support for at25sf128a flash * Fix function-name misspelling (#443) * Update host multicore.h to match multicore.h in rp2_common (#439) * Implement `uart_write_blocking` and `uart_read_blocking` for host (#438) * Define `__STRING` for other compilers than MSVC in the host platform.h file (#434) * Prevent warnings about some unused parameters in pico_stdio_usb when building with -Wextra (#431) * Fix warnings about some unused parameters in pico_stdio_usb * Use `__unused` for the unused parameter in tud_descriptor_configuration_cb * Remove redundant inclusions of `pico/platform.h` * Define `void operator delete[](void *p, std::size_t n)` in new_delete.cpp (#430) * queue: make data pointers const in queue_try_add and queue_add_blocking (#423) * misc interp_ fixes (#428) * some typo fixes (#408) * Prevent the literal string DEBUG from being appended to some messages in CMake < 3.15 (#433) * Add function to get the currently selected channel (#451) * Add missing board detection macros (#448) * add board detection macros for Sparkfun & RPi Pico / VGA Board * dma_channel_transfer_[from/to]_buffer_now: added const volatile to read_addr and volatile to write_addr (#449) * Change the quick-start instructions to include installation of the (#92) * added spi_get_baudrate() + some consistency changes (#395) * Allow lengthening xosc startup delay with a compile option (#457) * Add hardware_gpio accessors for Schmitt, slew rate, drive strength (fixes #290) (#464) * Add some spin lock related doxygen * Move to Tinyusb 0.10.0 (#462) * Add usb device dpram to svd file. Fixes #351 (#465) * Add PICO_PANIC_FUNCTION define to allow replacement of the default panic function (#463) * Add missing DREQ_ definitions * store actual clock frequency in clock_configure (fixes #368) * Fix hw_is_claimed, and add xxx_is_claimed APIs * Add some PIO irq helper methods * Add DMA channel IRQ status getter and clear methods * Implement the correct PIO IRQ status/clear methods (good to have methods here as the h/w interrupt registers are super confusing) * fix pico_multicore dependencies * add missing wrapper func __aeabi_f2d * Further DMA/PIO IRQ API cleanup (and review fixes) * add PICO_INT64_OPS_IN_RAM flag * fix qtpy rp2040 uart rx rev B (#466) * Move to tinyusb 0.10.1 (upstream tinyusb repo) ($467) * Add gpio_set_irqover to match inover/outover/oeover (fixes #265) (#470) Co-authored-by: Andrew Scheller <andrew.scheller@raspberrypi.com> Co-authored-by: Christian Flach <cmfcmf.flach@gmail.com> Co-authored-by: Luke Wren <wren6991@gmail.com> Co-authored-by: Earle F. Philhower, III <earlephilhower@yahoo.com> Co-authored-by: majbthrd <majbthrd@gmail.com> Co-authored-by: Peter Lawrence <12226419+majbthrd@users.noreply.github.com> Co-authored-by: Brian Cooke <bdscooke@gmail.com> Co-authored-by: Scott Shawcroft <scott@tannewt.org> Co-authored-by: Michael Stoops <spam@michaelstoops.com> Co-authored-by: ZodiusInfuser <christopher.parrott2@gmail.com> Co-authored-by: Kirk Benell <36707344+kirk-sfe@users.noreply.github.com> Co-authored-by: Ha Thach <thach@tinyusb.org> Co-authored-by: Exr0n <spotyie@gmail.com> Co-authored-by: Joni Kähärä <joni.kahara@async.fi> Co-authored-by: Rafael G. Martins <rafael@rafaelmartins.eng.br> Co-authored-by: Jonathan Reichelt Gjertsen <jonath.re@gmail.com> Co-authored-by: Martino Facchin <m.facchin@arduino.cc> Co-authored-by: Rene <reneg973@gmail.com> Co-authored-by: Brendan <2bndy5@gmail.com> Co-authored-by: geurtv <48989893+geurtv@users.noreply.github.com> Co-authored-by: ewpa <34030942+ewpa@users.noreply.github.com> Co-authored-by: Dan Halbert <halbert@halwitz.org> Co-authored-by: Liam Fraser <liam@raspberrypi.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
implementations to replace WFE/SEV with something more appropriate