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

Maximum Task Priority #687

Open
noahzarro opened this issue Jan 19, 2023 · 5 comments · Fixed by #690
Open

Maximum Task Priority #687

noahzarro opened this issue Jan 19, 2023 · 5 comments · Fixed by #690
Labels
risc-v Related to RISC-V
Milestone

Comments

@noahzarro
Copy link

The book states, that task priorities are in range 1..=(1 << NVIC_PRIO_BITS), where NVIC_PRIO_BITS could be up to 8 bits. The logical priority of the monotonic task however is set to 1 << NVIC_PRIO_BITS. In case of NVIC_PRIO_BITS == 8, this would lead to the logical priority being 256, i.e. 1 unit higher than allowed and an overflow would happen. This would make the timer the least prioritized interrupt.

I think the issue is not very urgent, since (to my limited knowledge) cortex-m devices normally do not offer 8 priority bits. However, I stumbled upon this when I was porting RTIC to the RISC-V processor architecture (with the CLIC extension), that offers 8 priority bits.

Best,
Noah

@AfoHT
Copy link
Contributor

AfoHT commented Jan 22, 2023

Exciting with RISC-V activities! :)

As you might have noted, in export.rs there's this function:

#[inline]
#[must_use]
pub fn logical2hw(logical: u8, nvic_prio_bits: u8) -> u8 {
    ((1 << nvic_prio_bits) - logical) << (8 - nvic_prio_bits)
}

The logical priorities are not quite the same as the hardware/NVIC priorities, as logical2hw handles the translation.

# Lowest RTIC priority: 1 
> ((1 << 8) - 1) << (8 - 8)

  shift(shift(1, 8) − 1, 8 − 8) = 255

# Highest RTIC priority: (1 << 8) = 256
> ((1 << 8) - (1 << 8)) << (8 - 8)

  shift(shift(1, 8) − shift(1, 8), 8 − 8) = 0

@korken89
Copy link
Collaborator

korken89 commented Jan 22, 2023

Thanks for finding, but the current system is correct! :)

For RISCV support we are officially adding it to RTIC 2.
If you feel like it you're welcome to help out work the design so supporting both arm and RISCV goes smoothly. :)

Plus the codegen is RTIC 2 is much simpler, so adding support should be close to trivial.
Ping me on matrix if you want to discuss!

@bors bors bot closed this as completed in 3240fb3 Jan 22, 2023
@noahzarro
Copy link
Author

Oh, very cool to hear that a new version is in the pipeline!

Yes, I see the logical to hardware transformation, but that's not what I wanted to say. I probably did not express myself properly :) What I meant was that the highest logical RTIC priority is 256, but the function #[must_use] pub fn logical2hw(logical: u8, nvic_prio_bits: u8) -> u8 takes only an u8 as the logical priority input. But 256 (the maximum logical RTIC priority) does not fit into a u8.

@AfoHT
Copy link
Contributor

AfoHT commented Jan 25, 2023

Now I see your point, good catch!

error: this literal must be in the range 1...255
  --> ui/task-priority-too-high.rs:46:37
   |
46 |     #[task(binds = I2C1, priority = 256)]
   |                                     ^^^

Trying to put prio 256 would at least not silently fail/overflow but rather explode and not let the user build :)

It simply does not fit in the u8 and Rust will let us know ❤️

What to do about it... I'm a bit hesitant to just throw in a larger type there, especially as there seems hardware having the use case is RISC-V (which we currently do not support in RTIC v1)

For RTIC v2 this should be kept in mind, so I'll reopen the task as a tracking issue for this :)

@AfoHT AfoHT reopened this Jan 25, 2023
@noahzarro
Copy link
Author

Great, this sounds reasonable :)

@AfoHT AfoHT added the risc-v Related to RISC-V label Jan 27, 2023
@AfoHT AfoHT added this to the v2.0.0 milestone Jan 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
risc-v Related to RISC-V
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants