Skip to content

Fix S2 systimers #1979

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.

Sign up for GitHub

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

Merged
MabezDev merged 10 commits into from
Aug 21, 2024
Merged

Fix S2 systimers #1979

Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
f4453ff
Add basic systimer interrupt tests
bugadani Aug 21, 2024
c38ddcc
Remove unnecessary condition
bugadani Aug 21, 2024
ff551b6
Fix edge interrupt bitmasks
bugadani Aug 21, 2024
804a040
Modify target_conf in critical section
bugadani Aug 21, 2024
d725936
Remove unnecessary fn call
bugadani Aug 21, 2024
7615979
Fix test
bugadani Aug 21, 2024
29d9b19
Add failing test case
bugadani Aug 21, 2024
e912eb0
Fix S2 systimer interrupts being fired unexpectedly
bugadani Aug 21, 2024
01661d0
Add changelog entry
bugadani Aug 21, 2024
f78817f
Format
bugadani Aug 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Modify target_conf in critical section
  • Loading branch information
@bugadani
bugadani committed Aug 21, 2024
commit 804a0402f93b57a33c512307a66dbb8835b816d6
12 changes: 6 additions & 6 deletions esp-hal/src/timer/systimer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,20 +396,20 @@ pub trait Comparator {
fn set_enable(&self, enable: bool) {
let systimer = unsafe { &*SYSTIMER::ptr() };

#[cfg(not(esp32s2))]
critical_section::with(|_| {
#[cfg(not(esp32s2))]
systimer.conf().modify(|_, w| match self.channel() {
0 => w.target0_work_en().bit(enable),
1 => w.target1_work_en().bit(enable),
2 => w.target2_work_en().bit(enable),
_ => unreachable!(),
});
});

#[cfg(esp32s2)]
systimer
.target_conf(self.channel() as usize)
.modify(|_r, w| w.work_en().bit(enable));
#[cfg(esp32s2)]
systimer
Copy link
Collaborator

@Dominaezzz Dominaezzz Aug 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why move this into the critical section? The register isn't shared on the S2.

Copy link
Contributor Author

@bugadani bugadani Aug 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The better question is, why wasn't this (and, like all other operations) in a critical section? Every method needs a shared reference only, what prevents users from shooting themselves in the foot by doing things in interrupts that they shouldn't?

Copy link
Collaborator

@Dominaezzz Dominaezzz Aug 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sharing with interrupts requires Sync, which Comparators aren't (or at least, shouldn't be).

The shared reference thing let's you give multiple objects on the same "thread" (interrupt counts as a separate thread) access to the object.

Copy link
Contributor Author

@bugadani bugadani Aug 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alarm is Sync as things currently stand :)

Copy link
Collaborator

@Dominaezzz Dominaezzz Aug 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah that needs fixing. Either by adding critical sections in Alarm or marking Alarms as not Sync.

The latter is easier (and maybe also the right way) since esp-wifi and esp-hal-embassy don't take advantage of the Sync, they only Send the alarms around.

Copy link
Contributor Author

@bugadani bugadani Aug 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would you prefer !Sync over &mut self methods?

Copy link
Collaborator

@Dominaezzz Dominaezzz Aug 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think &mut self is artificially restrictive. At the end of the day the volatile registers are basically glorified Cells.
I really don't want to have to pull out RefCell, Cell, etc. to share what is already a Cell amongst objects in the same thread.
imo, if an exclusive reference isn't needed it shouldn't be required.
If users want to enforce an invariant that requires exclusive access then they can use the Rust tool for that, either by taking ownership of the object or taking a mutable reference to the object. (This is done in FronzenUnit for example)

Side note there's also this part of the API GUIDELINES.

Avoiding &mut self when &self is safe to use. &self is generally easier to use as an API. Typical applications of this are where the methods just do writes to registers which don't have side effects.

(Though I suppose pointing it out doesn't matter since we're questioning the guideline itself)

.target_conf(self.channel() as usize)
.modify(|_r, w| w.work_en().bit(enable));
});
}

/// Returns true if the comparator has been enabled. This means
Expand Down