Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Xtensa CS implementation #90

Closed
wants to merge 5 commits into from
Closed

Xtensa CS implementation #90

wants to merge 5 commits into from

Conversation

MabezDev
Copy link
Member

@MabezDev MabezDev commented Jun 23, 2022

  • CS implementation for single core using PS.INTLEVEL
  • Added locking for dual core chips using a reetrant mutex. Reentrancy
    is important for nested critical sections.
  • Renames single_core & dual_core features to unicore & multicore respectively
  • CI: Run clippy on each chip so that cfg(target_arch = "...") paths are taken correctly
  • Remove redundant features

Questions

It's unclear what the INIT value for thread id should be, see the docs: https://docs.rs/lock_api/0.4.7/lock_api/trait.GetThreadId.html. I chose the first CPU as an init value, but perhaps this needs to be "not locked by either" value.

esp-hal-common/src/lib.rs Outdated Show resolved Hide resolved
esp-hal-common/src/lib.rs Outdated Show resolved Hide resolved
@MabezDev MabezDev force-pushed the feature/xtensa-cs-impl branch 6 times, most recently from 42731f7 to d66f074 Compare June 24, 2022 11:43
@MabezDev MabezDev marked this pull request as ready for review June 24, 2022 12:29
@MabezDev
Copy link
Member Author

Sort of ended up doing a few other things, sorry! Happy to split the other changes into another PR if its an issue :).

@MabezDev
Copy link
Member Author

On another note, I don't have a way of testing the multicore lock as esp-hal only supports core 1 on the esp32 & esp32s3

@bjoernQ
Copy link
Contributor

bjoernQ commented Jun 27, 2022

I tried to use this implementation in esp-wifi but failed at it. Because of the WIP status of esp-alloc I used commit cc67547 of esp-wifi and disabled my critical-section implementation.

I don't see how nested critical sections should work with this implementation without honoring the token in release

I got it to work with something clumsy as:

unsafe fn acquire() -> u8 {
            if VPS == 0 {
                core::arch::asm!("rsil {0}, 15", out(reg) VPS);
                #[cfg(feature = "multicore")]
                {
                    // let guard = multicore::MULTICORE_LOCK.lock();
                    // core::mem::forget(guard); // forget it so drop doesn't
                    // run
                }
                1
            } else {
                0
            }
        }

        unsafe fn release(_token: u8) {
            #[cfg(feature = "multicore")]
            {
                // debug_assert!(multicore::MULTICORE_LOCK.
                // is_owned_by_current_thread()); safety: we
                // logically own the mutex from acquire()
                // multicore::MULTICORE_LOCK.force_unlock();
            }
            if _token != 0 {
                core::arch::asm!("wsr.ps {0}", in(reg) VPS);
                VPS = 0;
            }
        }

But as soon as I comment-in the multicore stuff it doesn't work anymore.

Also, in my implementation I kept level 6 interrupts to make debugging work in CS (but that is probably not good in general)

But definitely a good idea to have support for critical sections in the HAL!

@MabezDev
Copy link
Member Author

I don't see how nested critical sections should work with this implementation without honoring the token in release

D'oh 🤦, got too obsessed with the reentrant multicore mutex that I forgot about making the interrupt-free part re-entrant. Thanks for that!

Also, in my implementation I kept level 6 interrupts to make debugging work in CS (but that is probably not good in general)

I think I will do the same, it's unlikely debug interrupts will interfere with normal code, and having debugging available is nice.

I will put back this PR to draft status until I fix the issues.

@MabezDev MabezDev marked this pull request as draft June 27, 2022 09:42
@bjoernQ
Copy link
Contributor

bjoernQ commented Jul 5, 2022

On another note, I don't have a way of testing the multicore lock as esp-hal only supports core 1 on the esp32 & esp32s3

#96 implements support for the second core

- CS implementation for single core using PS.INTLEVEL
- Added locking for dual core chips using a reetrant mutex. Reentrancy
  is important for nested critical sections.
- Update bare_metal to use git version with RefCellHelper
- Use master critical_section for configurable token size & the correct
  `CriticalSection` struct from `critical_section::with`
- Modify multicore s3 example to use `bare_metal::Mutex` &
  `critical_section`
@MabezDev MabezDev mentioned this pull request Jul 14, 2022
7 tasks
@jessebraham
Copy link
Member

What's the current status on this?

@MabezDev MabezDev mentioned this pull request Jul 20, 2022
3 tasks
@MabezDev MabezDev closed this Jul 20, 2022
@MabezDev MabezDev deleted the feature/xtensa-cs-impl branch November 8, 2022 22:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants