Skip to content

tests/multi_exmod/machine_can_05: Fix test setup race. - #19614

Open
projectgus wants to merge 1 commit into
micropython:masterfrom
projectgus:test/machine_can_05_race
Open

tests/multi_exmod/machine_can_05: Fix test setup race.#19614
projectgus wants to merge 1 commit into
micropython:masterfrom
projectgus:test/machine_can_05_race

Conversation

@projectgus

@projectgus projectgus commented Aug 13, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

This is an attempt to fix the problem described by Damien in this PR comment. It is a follow-up to b35383b which fixed one test setup race, but seemed to introduce failures - at least on some stm32 test configs.

Guessing at a root cause: the 100ms sleep may not have been enough for the host to always coordinate instances, in which case instance1 might start sending onto the bus before instance0. Fix is to add an explicit synchronisation point so instance1 doesn't try to send until after instance0 has definitely started "babbling".

This work was funded through GitHub Sponsors.

Testing

  • Testing current master between stm32 boards PYBDV11 and NUCLEO-F413ZH, on my system approx 1 in 100 runs of test machine_can_05_tx_prio_cancel.py would fail with similar output to @dpgeorge's report in the linked comment. Suspect host timing may be the reason why Damien sees this every time and I only see it occasionally.
  • With this fix, was able to complete 1000 test runs in both directions (-p2) with no failures.

Trade-offs and Alternatives

  • With this kind of fix it's sometimes unclear if you've solved the problem or merely changed some timing characteristics to move it, but using an explicit synchronisation point rather than sleeping 100ms and assuming this is long enough is unlikely to make things worse and may be fixing the root cause of the reported failures.

Generative AI

I did not use generative AI tools when creating this PR.

@projectgus
tests/multi_exmod/machine_can_05: Fix test setup race.
620f2e5 
Follow-up to b35383b which fixed one test setup race, but seemed to
introduce failures - at least intermittently on some stm32 test configs.

Guessing at a root cause: the 100ms sleep may not have been enough for the
host to always coordinate instances, in which case instance1 might start
sending onto the bus before instance0. Fix is to add an explicit
synchronisation point so instance1 doesn't try to send until after
instance0 has definitely started "babbling".

This work was funded through GitHub Sponsors.

Signed-off-by: Angus Gratton <angus@redyak.com.au>
@projectgus projectgus added the tests Relates to tests/ directory in source label Aug 13, 2026
@codecov

codecov Bot commented Aug 13, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.56%. Comparing base (2aa3966) to head (620f2e5).
⚠️ Report is 3 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master   #19614      +/-   ##
==========================================
- Coverage   98.59%   98.56%   -0.04%     
==========================================
  Files         182      182              
  Lines       23320    23320              
  Branches        5        5              
==========================================
- Hits        22992    22985       -7     
- Misses        327      334       +7     
  Partials        1        1
Flag Coverage Δ
unix-coverage-32bit 98.56% <ø> (-0.04%) ⬇️
unix-coverage-64bit 98.52% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@dpgeorge

dpgeorge commented Aug 13, 2026

Copy link
Copy Markdown
Member

Thanks for looking in to this.

Testing on PYBV10 <-> PYBV10:

  • without this PR I get about 10% failure rate on multi_extmod/machine_can_05_tx_prio_cancel.py (although it's pretty varied, I remember getting much more failures than that the other day!)
  • with this PR it now seems to be fully reliable: running it for 40 mins with -p2 -- 1200 tests in each direction -- I do not see any failures

With this kind of fix it's sometimes unclear if you've solved the problem or merely changed some timing characteristics to move it, but using an explicit synchronisation point rather than sleeping 100ms and assuming this is long enough

Agreed, but I think the fix here is good:

  • it replaces an ad-hoc delay with sync logic
  • the reason for the sync logic is clear, it makes sure everything is set up as expected and in the right order

So I think this we should merge this, although would be good if @robert-hh could test it to make sure it doesn't regress his set up.

@robert-hh

robert-hh commented Aug 13, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Actually it seems that with the alif board as instance 0 and the Teensy as instance 1 the fail rate is the same, 8 in 1000 vs. 8 in 1000 before. But the fail pattern may be different. I'll re-test with the previous version.
Note: This PR's version still skips the ALIF port.

Edit: The fail pattern is the same: Some low prio message manage to sneak in, at least with the Teensy as instance 1.

@robert-hh

robert-hh commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Other combinations:

PYBV11 and Alif AE3: 11 fails of 1000 attempts, Alif at instance 0
PYBV11 and Teensy: 0 fails of 1000, both directions (-p2).

So it looks like this change is good, and the behavior with the Alif port is independent.

robert-hh
robert-hh approved these changes Aug 13, 2026
View reviewed changes

@robert-hh robert-hh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Works fine. No regression. Test with PYBV11 and Teensys passes without fail. No increase in the fail rate with Alif AE3 and Teensy/PYBV11.

robert-hh
robert-hh reviewed Aug 13, 2026
View reviewed changes
Comment thread tests/multi_extmod/machine_can_05_tx_prio_cancel.py


def irq_recv(can):
global recv_std_id

@robert-hh robert-hh Aug 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This symbol is not used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dpgeorge dpgeorge Awaiting requested review from dpgeorge
1 more reviewer
@robert-hh robert-hh robert-hh approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

tests Relates to tests/ directory in source

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@projectgus @dpgeorge @robert-hh