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

ISO-TP: new ISO-TP implementation #7

Merged
merged 9 commits into from
Nov 14, 2023
Merged

ISO-TP: new ISO-TP implementation #7

merged 9 commits into from
Nov 14, 2023

Conversation

garethpotter
Copy link
Collaborator

  • single public context for send and receive; no more unbinding to send a message
  • only supports ISO-TP fixed addressing at present
  • broadly async approach, but sync receive supported for familiarity/compatibility
  • conformance tests ported from existing implementation

@garethpotter garethpotter force-pushed the isotp-fast branch 5 times, most recently from 95d03fa to f5fdfe9 Compare October 23, 2023 09:55
@martinjaeger
Copy link
Contributor

Thanks @garethpotter. Could you please rebase on main to resolve the conflicts?

@martinjaeger martinjaeger self-requested a review October 23, 2023 09:59
@garethpotter garethpotter force-pushed the isotp-fast branch 2 times, most recently from 7a8a2c4 to 9ca1e2f Compare October 23, 2023 11:02
garethpotter and others added 2 commits October 24, 2023 17:04
@garethpotter @martinjaeger
ISO-TP: new ISO-TP implementation
9786c55 
- single public context for send and receive; no more unbinding to send a message
- CAN-FD support
- only supports ISO-TP fixed addressing at present
- broadly async approach, but sync receive supported for familiarity/compatibility
- conformance tests ported from existing implementation
@martinjaeger
Tests: Integrate thingset_isotp_fast into existing can test
500ae24
martinjaeger
martinjaeger reviewed Oct 31, 2023
View reviewed changes
Copy link
Contributor

@martinjaeger martinjaeger left a comment

Choose a reason for hiding this comment

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

Looks very nice.

I have added a few commits to re-structure the tests and fix a bug. Now it is passing CI.

Some points to discuss when you are back:

  • We should use the terms CAN ID vs. ISO-TP address consistently. isotp_fast_msg_id is actually a CAN ID and variables of that type should not be named my_addr or recipient_addr. ISO-TP uses the terms source address and target address for the 1-byte section in the CAN ID.
  • Do we need thread synchronization to avoid race conditions when accessing the global slists?
  • Should we replace isotp_fast_context with isotp_fast_binding to avoid confusion with sctx and rctx?

src/isotp_fast.c Outdated
Comment on lines 849 to 850
sys_slist_init(&isotp_send_ctx_list);
sys_slist_init(&isotp_recv_ctx_list);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it safe to re-initialize the slist with every bind? And does this still allow to have multiple binds?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this did cross my mind at one point actually, but I never got round to doing anything about it. The lists should probably sit on the isotp_fast_context itself.

@garethpotter
Copy link
Collaborator Author

I have added a few commits to re-structure the tests and fix a bug. Now it is passing CI.

Noted. Thanks.

We should use the terms CAN ID vs. ISO-TP address consistently. isotp_fast_msg_id is actually a CAN ID and variables of that type should not be named my_addr or recipient_addr. ISO-TP uses the terms source address and target address for the 1-byte section in the CAN ID.

Yes, fair enough. I was trying to make it make more sense to me as I worked on it but it would definitely be better to change it back for consistency.

Do we need thread synchronization to avoid race conditions when accessing the global slists?

Possibly, but if these move to the isotp_fast_context then that becomes slightly less problematic.

Should we replace isotp_fast_context with isotp_fast_binding to avoid confusion with sctx and rctx?

I prefer context, because this is the only user-facing structure (replacing the existing send/receive contexts), but if you think there is real potential for confusion then let's rename it.

@martinjaeger
Copy link
Contributor

I prefer context, because this is the only user-facing structure (replacing the existing send/receive contexts), but if you think there is real potential for confusion then let's rename it.

Ok, fine with me.

@garethpotter
Copy link
Collaborator Author

I'm not quite sure what to do regarding locking yet. I agree that there ought to be something in place, but I'm not entirely sure where. As it is, we have a global buffer pool and global memory slabs for the contexts and access to these is not protected by locks. I did contemplate moving the pools and slabs to the isotp_fast_context (which would at least reduce the need for locking, if not eliminate it), but while defining memory slabs without the macro is easy, it is less easy to do that for pools, so I have shied away that for now.

martinjaeger
martinjaeger requested changes Nov 14, 2023
View reviewed changes
Copy link
Contributor

@martinjaeger martinjaeger left a comment

Choose a reason for hiding this comment

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

Thread sync should be fine now with the slists moved to the contexts, as we will only have one ISO-TP instance when used with ThingSet anyway.

Only got one remaining comment.

src/can.c Outdated
@@ -627,7 +778,7 @@ static void thingset_can_thread()
}

while (true) {
thingset_can_process_inst(&ts_can_single, K_FOREVER);
k_sleep(K_FOREVER);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the process function not be kept if CONFIG_ISOTP_FAST is not used?

Also when it's used we don't need the overhead and additional stack of the thread anymore, if it sits there and sleeps forever. We should use SYS_INIT instead.

martinjaeger
martinjaeger approved these changes Nov 14, 2023
View reviewed changes
Copy link
Contributor

@martinjaeger martinjaeger left a comment

Choose a reason for hiding this comment

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

Thanks for the update.

Let's go ahead and merge this exciting new feature.

@martinjaeger martinjaeger merged commit df1bc8f into ThingSet:main Nov 14, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martinjaeger martinjaeger martinjaeger approved these changes

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

Successfully merging this pull request may close these issues.
2 participants
@garethpotter @martinjaeger