Skip to content

Conversation

1atabey1
Copy link
Contributor

@1atabey1 1atabey1 commented Jan 3, 2023

Automatic detection would previously fail to detect socketcan interfaces if they had been renamed. This pr changes the detection such that it checks the identifier link/can in the output.

This is how outputs with renamed CANs look like on my machine:

4: ptcan: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UP mode DEFAULT group default qlen 10\    link/can
5: pcan: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UP mode DEFAULT group default qlen 10\    link/can
6: sensorcan: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UP mode DEFAULT group default qlen 10\    link/can
7: hilcan: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UP mode DEFAULT group default qlen 10\    link/can

I checked on different debian based systems and the identifier was always there so hope that's a generic thing, if someone has the capabilities to verify this please feel free to do so :)

@1atabey1 1atabey1 changed the title make socketcan if detection more generic make socketcan interface detection more generic Jan 3, 2023
@lumagi
Copy link
Collaborator

lumagi commented Jan 3, 2023

Sorry for the intrusion, I just saw your pull request. I checked the iproute2 and the kernel code. The type should be set to link/can for every CAN interface, the value is exported by the kernel. As an example, when the kernel creates and ISO-TP socket, it also checks if the interface is of type CAN. So this should be fairly save to use.
I noticed that ip link also offers a JSON output that is easier to parse and would do away with the string operations. I'm sorry to hijack this, I just thought that it could be made more robust with the JSON output. I have created a commit in my fork: lumagi@a5a7e10
Which version of Debian are you running? I checked the package index and all Debian / Ubuntu versions that have the minimum required Python version 3.7 also ship an iproute2 release > 4.14.0 (Changelog).

@zariiii9003
Copy link
Collaborator

I think using json is indeed more robust.

@1atabey1
Copy link
Contributor Author

1atabey1 commented Jan 5, 2023

Neat! Didn't know it provided that option, I agree that it's certainly the much nicer solution.
I am mainly running ubuntu 22.04 on our systems so it's for sure available in my usecase as well.

@lumagi how do we do this process wise? Do you just create a new pull request and I close this one or should I pull your changes into my current branch?

@lumagi
Copy link
Collaborator

lumagi commented Jan 6, 2023

I wouldn't have write permission on your pull request. That would make it a little complicated if there's review feedback. I opened another one in #1478.

@1atabey1 1atabey1 closed this Jan 6, 2023
@zariiii9003 zariiii9003 removed this from the Next Release milestone Feb 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants