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

Added 29bit ID support to cansniffer #88

Closed
wants to merge 2 commits into from

Conversation

coryjfowler
Copy link

Moved from CAN_BCM to CAN_RAW. I have most of the functionality restored. I just need to look into how CAN_RAW filtering works and add that ability.

cansniffer.c Outdated
@@ -60,7 +61,7 @@
#include <net/if.h>

#include <linux/can.h>
#include <linux/can/bcm.h>
//#include <linux/can/bcm.h>
Copy link
Member

Choose a reason for hiding this comment

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

Please remove and do not comment out.

cansniffer.c Outdated
void rx_delete (int fd, int id);
void print_snifline(int id);
//void rx_setup (int fd, unsigned long id);
//void rx_delete (int fd, unsigned long id);
Copy link
Member

Choose a reason for hiding this comment

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

same here

cansniffer.c Outdated
int handle_timeo(int fd, long currcms);
void writesettings(char* name);
void readsettings(char* name, int sockfd);

int idx = 0;
Copy link
Member

Choose a reason for hiding this comment

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

static int idx;

Copy link
Member

Choose a reason for hiding this comment

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

as it's a global it will be initialized to 0 automatically.

cansniffer.c Outdated
{
unsigned long f = ((struct snif*)elem1)->current.can_id;
unsigned long s = ((struct snif*)elem2)->current.can_id;
if (f > s) return 1;
Copy link
Member

Choose a reason for hiding this comment

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

don't put if and return in same line

Copy link
Member

Choose a reason for hiding this comment

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

Please use one Tab instead of 4 Spaces for indention.

cansniffer.c Outdated
return 0;
}


Copy link
Member

Choose a reason for hiding this comment

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

one newline is enough

cansniffer.c Outdated

if (binary) {

for (i=0; i<sniftab[id].current.can_dlc; i++) {
for (i=0; i<sniftab[position].current.can_dlc; i++) {
Copy link
Member

Choose a reason for hiding this comment

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

Please fix the spaces here, too.

cansniffer.c Outdated
for (i=0; i<sniftab[id].current.can_dlc; i++)
if ((color) && (sniftab[id].marker.data[i]) && (!(sniftab[id].notch.data[i])))
printf("%s%02X%s ", ATTCOLOR, sniftab[id].current.data[i], ATTRESET);
for (i=0; i<sniftab[position].current.can_dlc; i++)
Copy link
Member

Choose a reason for hiding this comment

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

and here

cansniffer.c Outdated
(sniftab[id].current.data[i] < 0x7F))
if ((color) && (sniftab[id].marker.data[i]) && (!(sniftab[id].notch.data[i])))
printf("%s%c%s", ATTCOLOR, sniftab[id].current.data[i], ATTRESET);
for (i=0; i<sniftab[position].current.can_dlc; i++)
Copy link
Member

Choose a reason for hiding this comment

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

and here

cansniffer.c Outdated
if (sockfd)
rx_setup(sockfd, i);
// if (sockfd)
// rx_setup(sockfd, i);
Copy link
Member

Choose a reason for hiding this comment

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

please remove

cansniffer.c Outdated
if (sockfd)
rx_delete(sockfd, i);
// if (sockfd)
// rx_delete(sockfd, i);
Copy link
Member

Choose a reason for hiding this comment

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

please remove

Copy link
Member

@marckleinebudde marckleinebudde left a comment

Choose a reason for hiding this comment

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

You probably want to squash this into the first commit.

Copy link
Member

@marckleinebudde marckleinebudde left a comment

Choose a reason for hiding this comment

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

Please don't comment code out, just remove it, if it's not used anymore.

cansniffer.c Outdated
@@ -814,3 +764,14 @@ void readsettings(char* name, int sockfd){
else
printf("unable to read setting file '%s'!\n", fname);
};

int sniftab_index(canid_t id){
Copy link
Member

Choose a reason for hiding this comment

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

static
and a space between ) {

Copy link
Member

Choose a reason for hiding this comment

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

Please move the function to the beginning of this file, so that you don'y need the forward declaration.

README.md Outdated
@@ -13,7 +13,7 @@ subsystem (aka SocketCAN):
* canplayer : replay CAN logfiles
* cansend : send a single frame
* cangen : generate (random) CAN traffic
* cansniffer : display CAN data content differences (just 11bit CAN IDs)
* cansniffer : display CAN data content differences (~~just 11bit CAN IDs~~ Not any more!)
Copy link
Member

Choose a reason for hiding this comment

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

Just remove the comment in the about the 11 bit. We've git to have a history of the source code.

Moved from CAN_BCM to CAN_RAW.  Restored basic functionality that BCM had natively.  There are likely bugs due to some non functionality between how the original program operated with CAN_BCM and now, but it is working on my bench with a NMEA 2000 GPS.

Update README.md

Restored add/remove ID

Restored ability to remove or add active CAN frames.  The CAN_ID must already exist in sniftab for this to work.

Update cansniffer.c

Fixed a potential for a segmentation fault to occur with the sniftab array.
Changed arbitrary max array value to defined name for flexibility with increasing the array size.
Other polish I forgot about.

Settings file operations work

Aligned settings file operations for use with 29bit IDs.
Instead of saving all 800 "slots" (or 11 bits IDs for the program as originally written), it now saves in use slots based on CAN ID and will load that ID into a slot for the program to potentially match upon.

Truncate when overwritting settings

Need to truncate settings file...

Update README.md

Update cansniffer.c

Cleaned up some unused commented code.
Corrected formatting, removed old commented code, made suggested code changes.

Update README.md
@coryjfowler
Copy link
Author

I hope that works... I'm not very fluent with 'git'.

Copy link
Member

@marckleinebudde marckleinebudde left a comment

Choose a reason for hiding this comment

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

Please squash all commits into one and adjust the commit message of that commit.

cansniffer.c Outdated
{
unsigned long f = ((struct snif*)elem1)->current.can_id;
unsigned long s = ((struct snif*)elem2)->current.can_id;
if (f > s) return 1;
Copy link
Member

Choose a reason for hiding this comment

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

Please use one Tab instead of 4 Spaces for indention.

int idx = 0;


int comp (const void * elem1, const void * elem2)
Copy link
Member

Choose a reason for hiding this comment

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

Please remove the trailing whitespace

printf("was only able to read until index %d from setting file '%s'!\n",
i, fname);
printf("was only able to read unti from setting file '%s'!\n", fname);

Copy link
Member

Choose a reason for hiding this comment

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

Please remove the trailing whitespace.

for (j=0; j<8 ; j++){
sprintf(buf, "%02X", sniftab[i].notch.data[j]);
write(fd, buf, 2);
}
write(fd, "\n", 1);
/* 7 + 16 + 1 = 24 bytes per entry */
/* 12 + 16 + 1 = 29 bytes per entry */
Copy link
Member

Choose a reason for hiding this comment

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

Please remove the trailing whitespace.

@@ -709,40 +696,50 @@ void readsettings(char* name, int sockfd){
int fd;
char fname[30] = SETFNAME;
char buf[25] = {0};
int i,j;
int pos,j;
Copy link
Member

Choose a reason for hiding this comment

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

Please add a space after the ,

@@ -754,3 +751,14 @@ void readsettings(char* name, int sockfd){
else
Copy link
Member

Choose a reason for hiding this comment

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

... and here, too.

@@ -754,3 +751,14 @@ void readsettings(char* name, int sockfd){
else
printf("unable to read setting file '%s'!\n", fname);
};

int sniftab_index(canid_t id){

Copy link
Member

Choose a reason for hiding this comment

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

Please remove that line.

cansniffer.c Outdated
@@ -814,3 +764,14 @@ void readsettings(char* name, int sockfd){
else
printf("unable to read setting file '%s'!\n", fname);
};

int sniftab_index(canid_t id){
Copy link
Member

Choose a reason for hiding this comment

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

Please move the function to the beginning of this file, so that you don'y need the forward declaration.

cansniffer.c Outdated
@@ -593,11 +578,12 @@ int handle_timeo(int fd, long currcms){

};

void print_snifline(int id){
void print_snifline(int slot){
Copy link
Member

Choose a reason for hiding this comment

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

Please make it static

cansniffer.c Outdated
@@ -489,45 +451,68 @@ int handle_keyb(int fd){
return 1; /* ok */
};

int handle_bcm(int fd, long currcms){
int handle_frame(int fd, long currcms){
Copy link
Member

Choose a reason for hiding this comment

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

Please make it static.

Copy link
Member

@marckleinebudde marckleinebudde left a comment

Choose a reason for hiding this comment

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

I'm sure you've fixed some of these in the second commit. If so, please feel free to ignore my rumblings on these. :D

@@ -200,21 +216,21 @@ int main(int argc, char **argv)
signal(SIGHUP, sigterm);
signal(SIGINT, sigterm);

for (i=0; i < 2048 ;i++) /* default: check all CAN-IDs */
for (i=0; i < MAX_SLOTS ;i++) /* default: enable all slots */
Copy link
Member

Choose a reason for hiding this comment

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

for (i=0; i < MAX_SLOTS; i++)

cansniffer.c Outdated
}
}
else {
if(frame.can_dlc == sniftab[pos].current.can_dlc)
Copy link
Member

Choose a reason for hiding this comment

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

if (

cansniffer.c Outdated
}
else {
if(frame.can_dlc == sniftab[pos].current.can_dlc)
for(i=0; i<frame.can_dlc; i++){
Copy link
Member

Choose a reason for hiding this comment

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

for (i = 0; i < frame.can_dlc; i++) {

cansniffer.c Outdated
else {
if(frame.can_dlc == sniftab[pos].current.can_dlc)
for(i=0; i<frame.can_dlc; i++){
if(frame.data[i] != sniftab[pos].current.data[i] ){
Copy link
Member

Choose a reason for hiding this comment

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

if (frame.data[i] != sniftab[pos].current.data[i]) {


sniftab[id].timeout = (timeout)?(currcms + timeout):0;
sniftab[pos].current = frame;
for (i=0; i < 8; i++)
Copy link
Member

Choose a reason for hiding this comment

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

for (i = 0; i < 8; i++)

}
if (buf[10] & 1) {
if (is_clr(pos, ENABLE)) {
do_set(pos, ENABLE);
Copy link
Member

Choose a reason for hiding this comment

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

if (buf[10] & 1 && is_clr(pos, ENABLE)) {

Copy link
Author

Choose a reason for hiding this comment

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

I feel that this would create undesired behavior unless I modify the 'else' that follows it.

cansniffer.c Outdated
if (sockfd)
rx_delete(sockfd, i);
if (is_set(pos, ENABLE)) {
do_clr(pos, ENABLE);
}
for (j=7; j>=0 ; j--){
Copy link
Member

Choose a reason for hiding this comment

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

for (j = 7; j >= 0; j--) {

buf[2*j+7] = 0; /* cut off each time */
sniftab[pos].notch.data[j] =
(__u8) strtoul(&buf[2*j+12], (char **)NULL, 16) & 0xFF;
buf[2*j+12] = 0; /* cut off each time */
Copy link
Member

Choose a reason for hiding this comment

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

buf[2 * j + 12] = 0

(__u8) strtoul(&buf[2*j+7], (char **)NULL, 16) & 0xFF;
buf[2*j+7] = 0; /* cut off each time */
sniftab[pos].notch.data[j] =
(__u8) strtoul(&buf[2*j+12], (char **)NULL, 16) & 0xFF;
Copy link
Member

Choose a reason for hiding this comment

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

You can drop the casts and the & 0xff.
strtoul(&buf[2 * j + 12], NULL, 16);

cansniffer.c Outdated
int i;

for(i = 0; i <= idx; i++)
if(id == sniftab[i].current.can_id)
Copy link
Member

Choose a reason for hiding this comment

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

if (id == sniftab[i].current.can_id)

@hartkopp
Copy link
Member

Dear @marckleinebudde and @coryjfowler ,
I've just followed your discussion and I'm personally pretty unsure, how to proceed with cansniffer.
The original approach was to use the entire CAN_BCM functionality to just display the differences filtered inside BCM socket (and therefore in kernel space).
With 29bit IDs this becomes unusable as it makes no sense to create 2^29 BCM-jobs in a socket in the kernel, right? Therefore I tried to merge some portions of candump (using CAN_RAW) and the output generation of cansniffer into a new implementation here: https://github.com/hartkopp/can-utils/commits/master/cansniffer.c
My idea was to support more than one CAN bus at a time, provide a different colorization for them, support CAN FD and sort the output by CAN interface OR by CAN ID.

Maybe my goals where just to ambitious that I got stuck with the implementation?!?

But we need a complete new concept to handle, sort (by ID) and compare incoming frames from CAN_RAW - and I wonder if using the CAN_RAW filters makes sense at all :-/

Any thoughts?

@coryjfowler
Copy link
Author

coryjfowler commented Aug 17, 2018

Hello @hartkopp,

Right now this implementation checks to see if an ID exists in the sniftab array. It will then either add it to an empty slot or check the existing slot for changes in the frame. If is a new ID or there are changes to the existing, it updates the struct in that slot like the original code. It will then sort the sniftab array by current.ID using 'qsort' so that it is displayed nicely in order with a simple for loop.

My aim for socket filtering is to help alleviate the program from handling a bus with potentially more than 2048 IDs in active use, but the direction here could also just be to change the value of MAX_SLOTS to something sane and recompile.

@asbachb asbachb mentioned this pull request Aug 28, 2018
@ChevySSinSD
Copy link

I am very interested in using cansniffer for some reverse engineering on my Chevy SS and the GMLAN uses 29 bit extended IDs. Any chance this update to support extended IDs will be merged into the master anytime soon?

@brandonros
Copy link

This isn't clear: does candump support 29-bit messages as it stands now? Not cansniffer, but candump?

@hartkopp
Copy link
Member

candump can deal with all kinds for CAN traffic (11bit / 29bit Identifiers, Classic CAN and CAN FD). cansniffer can only cope with 11bit IDs and Classic CAN (8 bytes payload).

@mitchdetailed
Copy link

The method I used to get around this is to set up a vcan network and mask the id to the 11b value, and send it onto vcan0.

Example shown here: https://github.com/mitchdetailed/python-can-re/blob/master/can0ext-vcan1masked.py

@peteg944
Copy link

Still no progress? It's been a year and a half... RIP. Are there any alternatives to cansniffer that work with the full-length IDs?

@coryjfowler
Copy link
Author

Still no progress? It's been a year and a half... RIP. Are there any alternatives to cansniffer that work with the full-length IDs?

The version on my github has the functionality. However, it has not been accepted here because of styling issues that existed before I even made changes and I simply do not have the time to update those. So its being held back by purely petty reasons...

@hartkopp
Copy link
Member

Would you like to test some 29bit cansniffer rework?
See #181

BR,
Oliver

@hartkopp
Copy link
Member

Followed up in issue #181
Integrated this suggested slot approach from @coryjfowler with some more fixes and improvements.
@coryjfowler @SSinSD @brandonros @mitchdetailed @peteg944 :
You are invited to test (and review) this pull request: #200
The latest code can be found here: https://github.com/hartkopp/can-utils/tree/29bit-cansniffer
The single steps of development and improvements can be found here: https://github.com/hartkopp/can-utils/tree/snif

@hartkopp hartkopp closed this Apr 30, 2020
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.

None yet

7 participants