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.

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

Fix: Assign bitrate and loopback on for Socketcan #662

Merged
merged 2 commits into from
Aug 30, 2023

Conversation

baconYao
Copy link
Contributor

Description

Two changes are made in this commit.

  1. For most of devices with Can hardware component, the Can interface cannot be brought up if the bitrate is not given. Therefore, I assign the bitrate for eff and sff local socketcan tests like what the remote socketcan tests do.
  2. For local Socketcan test, loopback is the must need parameter be assigned before bringing the Can interface up.

Resolved issues

No Bitrate & No Loopback on:

With Bitrate and Loopback on:

Documentation

N/A

Tests

Two changes are made in this commit.
1. For most of devices with Can hardware component, the Can interface
   cannot be brought up if the bitrate is not given. Therefore, I
   assign the bitrate for eff and sff local socketcan tests like what
   the remote socketcan tests do.
2. For local Socketcan test, loopback is the must need parameter be
   assigned before bringing the Can interface up.
@baconYao baconYao requested a review from pieqq August 13, 2023 15:07
Copy link
Collaborator

@pieqq pieqq left a comment

Choose a reason for hiding this comment

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

I don't have much knowledge on the topic, so it's hard for me to review this, but thankfully you provide some submissions!

Where do the values you provide come from? For some of the commands, you issue a bitrate of 100,000 while for another one you set it to 1,000,000. Is this expected, or is it a typo?

If you have a source for the numbers you are using, could you add it in the commit message so that we know why we have these numbers?

Also, was this tested on the devices that were running these socketcan tests previously?

@codecov
Copy link

codecov bot commented Aug 29, 2023

Codecov Report

❗ No coverage uploaded for pull request base (main@9f16d82). Click here to learn what that means.
The diff coverage is n/a.

@@          Coverage Diff           @@
##             main    #662   +/-   ##
======================================
  Coverage        ?   1.95%           
======================================
  Files           ?     126           
  Lines           ?   14950           
  Branches        ?    2583           
======================================
  Hits            ?     292           
  Misses          ?   14595           
  Partials        ?      63           
Flag Coverage Δ
provider-base 1.93% <0.00%> (?)
providers 1.93% <0.00%> (?)
unittests 1.93% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

@baconYao
Copy link
Contributor Author

baconYao commented Aug 29, 2023

I don't have much knowledge on the topic, so it's hard for me to review this, but thankfully you provide some submissions!

Where do the values you provide come from? For some of the commands, you issue a bitrate of 100,000 while for another one you set it to 1,000,000. Is this expected, or is it a typo?

If you have a source for the numbers you are using, could you add it in the commit message so that we know why we have these numbers?

Also, was this tested on the devices that were running these socketcan tests previously?

The value 100,000 was used in lots of Project Checkbox like I listed above (Baoshan, Limeric, Baytwon and others). But to be honest, I have no idea why they were 100,000 instead of 1,000,000.

The value 1,000,000 is referenced from socketcan remote job.

Copy link
Collaborator

@pieqq pieqq left a comment

Choose a reason for hiding this comment

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

These changes have been verified on different devices from different projects using socketcan and are known to work, so all good.

+1

@pieqq pieqq merged commit 74cc03b into canonical:main Aug 30, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants