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

can/isotp: fix closing connection and improve flow control handling #12051

Merged
merged 2 commits into from
Sep 12, 2019
Merged

can/isotp: fix closing connection and improve flow control handling #12051

merged 2 commits into from
Sep 12, 2019
+41 −25

Conversation

vincent-d
Copy link
Member

Contribution description

This PR fixes closing an ISO TP connection if the user-supplied timeout is shorter than CAN_ISOTP_TIMEOUT_N_Ar. In short, xtimers were not removed properly when closing.

This also add the ability to customize the flow control frame from the upper layers. We discovered some weird behavior with some vehicles that do not implement the protocol properly, so tweaking the flow control for a given connection can be useful.

Testing procedure

The bug was reproduced with the given configuration:

  1. RIOT node opens a connection and waits for reception with a timeout < 1s
  2. remote node sends a "first frame"
  3. RIOT node sends the "flow control"
  4. remote node doesn't send a "consecutive frame" -> timeout on the RIOT node, crash after CAN_ISOTP_TIMEOUT_N_Ar s (1s)

Vincent Dupont added 2 commits August 21, 2019 11:16
can/isotp: remove timers when closing connection
32b0070 
Timers were not properly removed leading to hard faults if it fires
after the connection is closed.
can/isotp: allow changing FC options at runtime
af66fe1
@aabadie aabadie added Area: network Area: Networking Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Aug 22, 2019
@tcschmidt tcschmidt requested a review from smlng August 24, 2019 13:10
@smlng smlng added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Sep 6, 2019
smlng
smlng approved these changes Sep 6, 2019
View reviewed changes
Copy link
Member

@smlng smlng left a comment

Choose a reason for hiding this comment

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

looks good to me

@benpicco benpicco merged commit 66ce29d into RIOT-OS:master Sep 12, 2019
@kb2ma kb2ma added this to the Release 2019.10 milestone Sep 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smlng smlng

Assignees
No one assigned
Labels
Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Milestone
Release 2019.10
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@vincent-d @smlng @benpicco @aabadie @kb2ma