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.

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

Jump to bottom

Add autostart option to BusABC.send_periodic() to fix issue #1848 #1853

Merged
merged 13 commits into from
Oct 8, 2024
Merged

Add autostart option to BusABC.send_periodic() to fix issue #1848 #1853

merged 13 commits into from
Oct 8, 2024

Conversation

SWolfSchunk
Copy link
Contributor

I have fixed my own issue #1848

hardbyte
hardbyte approved these changes Sep 8, 2024
View reviewed changes
Copy link
Owner

@hardbyte hardbyte left a comment

Choose a reason for hiding this comment

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

Seems reasonable

bures and others added 4 commits September 9, 2024 11:39
@bures @SWolfSchunk
Fix for hardbyte#1849 (PCAN fails when PCAN_ERROR_ILLDATA is read via…
efb0f94 
… ReadFD) (hardbyte#1850)

* When there is an invalid frame on CAN bus (in our case CAN FD), PCAN first reports result PCAN_ERROR_ILLDATA and then it send the error frame. If the PCAN_ERROR_ILLDATA is not ignored, python-can throws an exception.

This fix add the ignore on the PCAN_ERROR_ILLDATA.

* Fix for ruff error `can/interfaces/pcan/pcan.py:5:1: I001 [*] Import block is un-sorted or un-formatted`
Added comment explaining why to ignore the PCAN_ERROR_ILLDATA.
@SWolfSchunk
Merge branch 'main' into send_periodic_autostart
05444a9
@SWolfSchunk
Format with black to pass checks
9360118
@SWolfSchunk
Merge branch 'main' into send_periodic_autostart
4b8b84c
@SWolfSchunk
Copy link
Contributor Author

It seems like the test "testProducerConsumer" fails randomly on random pypy versions (3.9, 3.10) on random platforms (windows here so far, but other requests show the same behavior on ubuntu and macos).
I can not appraise if it is really a problem depending on the changes of other requests but here i am sure it is not coming from the changes I made. Maybe I should open a bug report fort that but maybe this is a problem of the threading implementation of pypy?

@zariiii9003
Copy link
Collaborator

The autostart parameter is ignored by socketcan and ixxat, right? This should either be fixed or documented.
And yes, the PyPy tests are flaky, it's probably not related to your changes.

@SWolfSchunk
Copy link
Contributor Author

SWolfSchunk commented Sep 17, 2024 •
edited
Loading

No that's not right the autostart parameter is not ignored by neither socketcan nor ixxat. The changes are all done on the internal implementations as well. Also everything changed is documented in the docstrings (that existed before).
Generally this changes should not break any old calls on send_periodic() at all (fully backward compatible) but extend it to the use case of my issue.

@zariiii9003
Copy link
Collaborator

zariiii9003 commented Sep 18, 2024 •
edited
Loading

No that's not right the autostart parameter is not ignored by neither socketcan nor ixxat.

Your change only affects the thread-based fallback. However, the socketcan broadcast manager starts immediately.

@SWolfSchunk
Copy link
Contributor Author

Hopefully I have fixed the missing parts now. Thanks for your hint @zariiii9003 .
I am still not sure about the changes for socketcan devices.
From how i read the code of its CyclicSendTask.init() instead of other devices they where not autostarting before why i set the default value to autostart=False in this case for backward compatibility.

@phettberg phettberg mentioned this pull request Sep 29, 2024
Closed
zariiii9003
zariiii9003 requested changes Oct 7, 2024
View reviewed changes
can/bus.py Outdated Show resolved Hide resolved
can/bus.py Outdated Show resolved Hide resolved
can/interfaces/socketcan/socketcan.py Outdated Show resolved Hide resolved
can/interfaces/socketcan/socketcan.py Outdated Show resolved Hide resolved
can/interfaces/socketcan/socketcan.py Outdated Show resolved Hide resolved
@SWolfSchunk SWolfSchunk closed this Oct 7, 2024
@SWolfSchunk SWolfSchunk reopened this Oct 7, 2024
@zariiii9003 zariiii9003 merged commit 7a3d23f into hardbyte:main Oct 8, 2024
33 checks passed
@zariiii9003
Copy link
Collaborator

Thank you 👍

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

@zariiii9003 zariiii9003 zariiii9003 approved these changes

@grant-allan-ctct grant-allan-ctct Awaiting requested review from grant-allan-ctct

@hardbyte hardbyte Awaiting requested review from hardbyte

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@SWolfSchunk @zariiii9003 @hardbyte @grant-allan-ctct @bures