interface for changing bitrate #1030
Conversation
implemented in vector. will add to socketcan eventually. just want to get some thoughts on this interface
Codecov Report
@@ Coverage Diff @@
## develop #1030 +/- ##
===========================================
- Coverage 66.51% 64.38% -2.13%
===========================================
Files 78 79 +1
Lines 7561 7651 +90
===========================================
- Hits 5029 4926 -103
- Misses 2532 2725 +193 |
|
Wouldn't it be nice to implement a more generic |
can/interfaces/vector/canlib.py
Outdated
| @@ -515,6 +516,15 @@ def handle_canfd_event(self, event: xlclass.XLcanRxEvent) -> None: | |||
| def send(self, msg: Message, timeout: typing.Optional[float] = None): | |||
| self._send_sequence([msg]) | |||
|
|
|||
| @property | |||
| def bitrate(self) -> int: | |||
| return self.bitrate | |||
There was a problem hiding this comment.
Doesn't this recuse infinitely? Usually, I see this being implemented with a private attribute self._bitrate.
There was a problem hiding this comment.
oops yeah I'll get that fixed. Just tossed this together quick to get some feedback.
|
Your proposal for implementing it as a property looks really nice! However, as a change to the core class of the library this should get more feedback. (Side note: This could also be used in other places since it modularizes the configuration of the bus. Sometimes, that might make things more readable.) |
|
Some questions:
|
I guess we would not have to implement it right away. Besides, wouldn't the bitrate be handled automatically?
I'd propose to simply throw one of the new exceptions, like a |
implement bitrate for socketcan using libsocketcan2. could just use ip and parse output to reduce dependency
This will only set the arbitration bitrate. Would probably need a separate property for the data bitrate.
Check out the latest commit. I think it should handle these issues. I still need to add unit tests but testing from home this all works fine. I'll need to test on my work setup too next week. |
| @@ -21,6 +21,29 @@ | |||
| log_tx = log.getChild("tx") | |||
| log_rx = log.getChild("rx") | |||
|
|
|||
| can_config_lib = ctypes.util.find_library("socketcan") | |||
There was a problem hiding this comment.
Simpler to use this library rather than parsing output from ip but adds a dependency. Thoughts?
| @@ -233,6 +233,8 @@ def __init__( | |||
| ) | |||
|
|
|||
| if permission_mask.value == self.mask: | |||
| self._init_access = True | |||
There was a problem hiding this comment.
I'm not a huge fan of this flag but I would rather not check for init access everytime.
The bitrate setting of 'real' CAN interfaces can only be performed as network-admin (aka root) on Linux. That's why the And the setting affects all users on the system (not only python-can users), as the interface has to be shut down to change the bitrates (and has to be set to "up" afterwards). Shutting down the interfaces leads to -ENETDOWN return values for all user space applications working on that interface. Additionally the python-can application needs to be run as root to manipulate the interface status and the bitrate. So reading the interface properties via |
|
@hartkopp Thank you for your input. It's always neat to have the expert here 👍 |
@marckleinebudde : Is there any ongoing activity planned for |
|
I agree with @hartkopp. The executing process must be root or at least have the |
|
There is no immediate activity planned for |
Quick implementation of changing the bitrate at runtime. I just want to get a feel if this is the interface we would like or if there is something better.
I just implemented the Vector interface for now but I can add socketcan eventually (would close #549 ).
This might be a start for implementing autobaud as well.
TODO