Relax restriction of arbitration ID uniqueness for SocketCAN #785

tysonite · 2020-03-02T17:01:41Z

References #721
I am unsure about the correct way to make a unique identifier. Please, suggest.

codecov · 2020-03-02T17:10:23Z

Codecov Report

Merging #785 into develop will increase coverage by 0.07%.
The diff coverage is 95.23%.

@@             Coverage Diff             @@
##           develop     #785      +/-   ##
===========================================
+ Coverage    69.50%   69.58%   +0.07%     
===========================================
  Files           70       70              
  Lines         6556     6565       +9     
===========================================
+ Hits          4557     4568      +11     
+ Misses        1999     1997       -2

karlding · 2020-03-05T05:43:54Z

can/interfaces/socketcan/socketcan.py

+GLOBAL_TASK_ID = 0
+TASK_LOCK = threading.Lock()


Instead of making this global, perhaps consider storing this on the Bus context since we map 1:1 to a SocketCAN bus. This is because the kernel code matches based on the combination of:

Task ID (op->can_id)

Interface (op->ifindex)

Flags (op->flags)

/* * helpers for bcm_op handling: find & delete bcm [rx|tx] op elements */ static struct bcm_op *bcm_find_op(struct list_head *ops, struct bcm_msg_head *mh, int ifindex) { struct bcm_op *op; list_for_each_entry(op, ops, list) { if ((op->can_id == mh->can_id) && (op->ifindex == ifindex) && (op->flags & CAN_FD_FRAME) == (mh->flags & CAN_FD_FRAME)) return op; } return NULL; }

(Taken from the 5.5.7 kernel)

I think this should be doable if we expand the socketcan.CyclicTask to take an ID argument in __init__. Thoughts?

This looks good to me. I am thinking about adding a member function to SocketcanBus class that returns next task ID, incrementing old one. The result of that function passed to CyclicSendTask::__init__ as an additional argument. I will try that.

karlding · 2020-03-05T05:44:16Z

can/interfaces/socketcan/socketcan.py

+    with TASK_LOCK:
+        global GLOBAL_TASK_ID
+        GLOBAL_TASK_ID += 1
+        return GLOBAL_TASK_ID


We will want to also ensure that we don't overflow the canid_t type, on the off-chance that someone is running this for extended periods of time and repeatedly starting/stopping periodic TX tasks.

typedef __u32 canid_t;

Ensuring that we wrap at 2^11 - 1 is probably sufficient for now.

Thanks, good catch. It looks to be an unsigned integer though, thus wrapping is required at 2^32 - 1, isn't it?

Considering this accidental case, some of the previous tasks IDs can be used already, wouldn't it better still implement usage of CAN_BCM_TX_READ to check if they are busy or not?

When I commented, I wasn't sure if the kernel code was doing anything with anything beyond the 11-bits (ie. checking other bits for various flags in the 32-bits) so I just said 11-bits to be safe to avoid any potential unintended side effects.

Looking more closely through the kernel code now, it seems like for TX Tasks, this value is only copied (for things other than the TX Task ID) when TX_CP_CAN_ID is set as part of flags on the BCM message. The only places it's used for determining behaviour is when dealing with RX tasks, but they're stored in separate lists so they won't affect each other (and python-can currently doesn't support this).

But yes, you're right, it should be safe to increase this to 32-bits.

Considering this accidental case, some of the previous tasks IDs can be used already, wouldn't it better still implement usage of CAN_BCM_TX_READ to check if they are busy or not?

Yes, let's do that. If we detect that the TX ID has been used, let's just raise an Exception, and make it explicit for the user to decide whether to retry calling bus.send_periodic.

karlding · 2020-03-05T05:44:34Z

test/test_cyclic_socketcan.py

-
-        # The second one raises a ValueError when we attempt to create a new
-        # Task, since it has the same arbitration ID.
-        with self.assertRaises(ValueError):


I think this test could probably be repurposed to ensure that we receive both messages, rather than throwing an exception.

Modified this test.

karlding · 2020-03-15T00:37:05Z

can/interfaces/socketcan/socketcan.py

        return task

+    def _get_next_task_id(self) -> int:
+        self._task_id = (self._task_id + 1) % (2 ** 11 - 1)


I believe you still need mutual exclusion around the task_id since access to the Bus via send_periodic isn't serialized. I think we assume that send_periodic can be called concurrently.

I am not really sure in that. There is ThreadSafeBus, but I don't really understand whether it serializes access to periodic messages sending. Anyway, I've added a guard.

can/interfaces/socketcan/socketcan.py

karlding

LGTM % the missing documentation comment. Thanks for working on this!

hardbyte

This is looking really solid. 🚀

Do you mind updating the interfaces/socketcan and bcm documentation too?

tysonite · 2020-04-08T22:17:50Z

I thought about that too. No problem.

I would be happy to update if you point out what you want to add into existing documentation. Note please that those changes are not user-visible, public API hasn't changed.

But I could add some description that clarifies internals, though. Is that what you want?

hardbyte · 2020-04-09T00:14:01Z

Awesome thanks, clarifying the internal behavior is what I was thinking. But you are now in the best position to check that the docstrings are still accurate e.g. socketcan.CyclicSendTask.

tysonite · 2020-04-09T14:24:25Z

I did few more changes to Sphinx docs if you don't mind along with other updates to docstrings.

doc/interfaces/socketcan.rst

hardbyte

🌟 for extra doc updates. Thank you for improving python-can!

tysonite · 2020-04-10T07:30:22Z

@karlding, @hardbyte, thank you for review and letting that in.

tysonite added 2 commits March 2, 2020 19:34

relax restriction on unique arb_id for socketcan bcm

e559e1a

remove test_create_same_id_raises_exception

0a3672d

karlding reviewed Mar 5, 2020

View reviewed changes

tysonite added 3 commits March 5, 2020 19:09

handle tasks id differently

9d7c599

Merge branch 'develop' into equal_arbids

03c5f7c

wrap task id

1118e3b

tysonite requested a review from karlding March 9, 2020 20:18

karlding reviewed Mar 15, 2020

View reviewed changes

Add a check that task id is not already present

c724519

tysonite requested a review from karlding April 4, 2020 13:32

karlding reviewed Apr 6, 2020

View reviewed changes

can/interfaces/socketcan/socketcan.py Show resolved Hide resolved

karlding reviewed Apr 6, 2020

View reviewed changes

tysonite and others added 2 commits April 7, 2020 18:56

Add task_id description

3f3a9ba

Merge branch 'develop' into equal_arbids

e169f99

hardbyte approved these changes Apr 8, 2020

View reviewed changes

karlding approved these changes Apr 9, 2020

View reviewed changes

Update docs

53c25db

Update docs

c2946e7

hardbyte reviewed Apr 9, 2020

View reviewed changes

doc/interfaces/socketcan.rst Outdated Show resolved Hide resolved

hardbyte approved these changes Apr 9, 2020

View reviewed changes

minor rephrasing in doc/interfaces/socketcan.rst

f67a780

hardbyte merged commit 2d672b3 into hardbyte:develop Apr 9, 2020

tysonite deleted the equal_arbids branch April 18, 2020 13:49

tysonite mentioned this pull request Apr 23, 2020

Release 3.3.3 #715

Merged

tysonite mentioned this pull request Mar 15, 2021

socketcan cannot create multiple periodic messages with same arb id #930

Closed

felixdivo added this to the 4.0.0 Release milestone Apr 16, 2021

felixdivo added backend:socketcan os:linux labels Apr 16, 2021

sschueth mentioned this pull request Jan 24, 2024

Added VariableRateCyclicTaskABC and updated ThreadBasedCyclicSendTask #1733

Open

Relax restriction of arbitration ID uniqueness for SocketCAN #785

Relax restriction of arbitration ID uniqueness for SocketCAN #785

Uh oh!

Conversation

tysonite commented Mar 2, 2020

Uh oh!

codecov bot commented Mar 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tysonite Apr 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

karlding left a comment

Choose a reason for hiding this comment

Uh oh!

hardbyte left a comment

Choose a reason for hiding this comment

Uh oh!

tysonite commented Apr 8, 2020

Uh oh!

hardbyte commented Apr 9, 2020

Uh oh!

tysonite commented Apr 9, 2020

Uh oh!

Uh oh!

hardbyte left a comment

Choose a reason for hiding this comment

Uh oh!

tysonite commented Apr 10, 2020

Uh oh!

Uh oh!

codecov bot commented Mar 2, 2020 •

edited

Loading

tysonite Apr 4, 2020 •

edited

Loading