RealCAN implementation #521
Conversation
|
Hello, Let us know if there's anything more to tweak. Cheers |
|
Please squash everything into one commit. |
Replay pre-recorded CAN Bus dumps respecting the original relative timestamps. Fix code style Conform to codebase style as per maintainer's suggestions. Refactor code logic according to suggestions for PR linux-can#521
Done! 👍 |
|
please rebase to latest master, which includes #522 and causes a conflict now. |
|
If I create a logfile with
|
canplayer.c
Outdated
| @@ -252,6 +261,38 @@ static int add_assignment(char *mode, int socket, char *txname, | |||
| return 0; | |||
| } | |||
|
|
|||
| void load_gaps_from_file(FILE *fp, struct sleep *timestamps) | |||
There was a problem hiding this comment.
| void load_gaps_from_file(FILE *fp, struct sleep *timestamps) | |
| static void load_gaps_from_file(FILE *fp, struct sleep *timestamps) |
canplayer.c
Outdated
| free(line); | ||
| } | ||
|
|
||
| void init_timestamps(struct sleep *timestamps, size_t init_size) |
There was a problem hiding this comment.
| void init_timestamps(struct sleep *timestamps, size_t init_size) | |
| static void init_timestamps(struct sleep *timestamps, size_t init_size) |
canplayer.c
Outdated
|
|
||
| while ((read = getline(&line, &len, fp)) != -1) { | ||
| if (timestamps->idx == timestamps->size) { | ||
| timestamps->size++; |
There was a problem hiding this comment.
It might be beneficial to double the size instead of incrementing. How many lines does your usual files have?
canplayer.c
Outdated
| exit(1); | ||
| } | ||
| } | ||
| sscanf(line, "(%lu.%lu)", ×tamps->sleep_vector[timestamps->idx].tv_sec, ×tamps->sleep_vector[timestamps->idx].tv_usec); |
There was a problem hiding this comment.
nitpick: an intermediate variable, e.g. tv = ×tamps->sleep_vector[timestamps->idx] might be beneficial here.
canplayer.c
Outdated
| timestamps->size++; | ||
| timestamps->sleep_vector = realloc(timestamps->sleep_vector, timestamps->size * sizeof(struct timeval)); | ||
| if (!timestamps->sleep_vector) { | ||
| fprintf(stderr, "Failed to create the timestamps vector!\n"); |
There was a problem hiding this comment.
| fprintf(stderr, "Failed to create the timestamps vector!\n"); | |
| fprintf(stderr, "Failed to realloc the timestamps vector!\n"); |
canplayer.c
Outdated
| while ((read = getline(&line, &len, fp)) != -1) { | ||
| if (timestamps->idx == timestamps->size) { | ||
| timestamps->size++; | ||
| timestamps->sleep_vector = realloc(timestamps->sleep_vector, timestamps->size * sizeof(struct timeval)); |
There was a problem hiding this comment.
| timestamps->sleep_vector = realloc(timestamps->sleep_vector, timestamps->size * sizeof(struct timeval)); | |
| timestamps->sleep_vector = realloc(timestamps->sleep_vector, timestamps->size * sizeof(*(timestamps->sleep_vector)); |
canplayer.c
Outdated
| gap_from_file = true; /* using time delta from file */ | ||
| init_timestamps(×tamps, 1); | ||
| load_gaps_from_file(infile, ×tamps); | ||
| timestamps.idx = 0; |
There was a problem hiding this comment.
| timestamps.idx = 0; |
You already do this in init_timestamps().
|
Ok, I understand which problem you want to solve. The current code sleeps for Properly fixing this requires several steps on the existing code:
See f55ea38, where I added absolute timing to |
I have a general question about this patch. So what functionality does your patch add? |
|
|
|
Sounds good, the changes you suggested enable a more precise replay of the original trace. The use of the However, during our developments we could not saturate the bus, be it a virtual We overcame this limitation using a spin-lock while waiting for a given deadline. Comparing our implementation against the upstream Thus, the Since the paper is still under review, we can only share the results privately at the moment. Let us know what you think about this evaluation, while we apply the minor correction stil pending. |
|
If the overhead by Can you illustrate in a few lines of shell code, how the Works: Adding Maybe it hangs in: |
Replay pre-recorded CAN Bus dumps respecting the original relative timestamps. Fix code style Conform to codebase style as per maintainer's suggestions. Refactor code logic according to suggestions for PR linux-can#521
|
e49499d throws the following warnings on amd64: on 32 bit arm: |
Replay pre-recorded CAN Bus dumps respecting the original relative timestamps. Fix code style Conform to codebase style as per maintainer's suggestions. Refactor code logic according to suggestions for PR linux-can#521
|
I still have a problem with the new restriction, that requires a logfile and does not work with E.g. a common use-case to use a "remote" CAN interface on your local host is: When looking at this code, which seems to be the relevant difference to the former nanosleep approach: Why shouldn't this work with |
|
If you read the timestamps into the array before stating the sending loop you need to rewind the input stream, this is not possible with stdin. Yes, it makes no sense to read the timestamps in the first place. And the |
Replay pre-recorded CAN Bus dumps respecting the original relative timestamps. Fix code style Conform to codebase style as per maintainer's suggestions. Refactor code logic according to suggestions for PR linux-can#521
|
I agree with marckleinebudde that is not possible to implement real time sending from stdin. This solution does not introduce any delay in the sending phase. |
|
The question remains, why do you need to read all the time stamps before into the array? And it's still not clear how the new option should be used: It only starts this way: I'm trying to replay candump-absolute.log. A CAN frame should be send every 100ms, but it doesn't work as expected: |
|
Additionally the canplayer has to be able to cope with timestamps that go back and forth as the timestamps created with The question to me is whether we can increase the accuracy on demand with some improvement that goes away from the "gap" approach. |
|
See: #521 (comment) |
Replay pre-recorded CAN Bus dumps respecting the original relative timestamps. Fix code style Conform to codebase style as per maintainer's suggestions. Refactor code logic according to suggestions for PR linux-can#521
|
Since the -r option check the presence of an input file, the only way to use is I implemented the solution you suggested in #521 (comment) removing the pre-processing phase while keeping the main loop implementation using the Thank you for the advise. |
|
Of course you name the variable to switch to the real time mode |
|
I think that the variable name should remain keep in mind that reading the messages directly from the |
|
I'm still not convinced. You also have a busy loop while() statement in line 588 here: So where is the improvement to the former loop? If so - are you aware of the E.g. you can define the gap length for the main loop even smaller than 1ms:
which reduces the while loop to 10 micro seconds in this case and therefore increases the precision for the replay timestamps. Does this probably fit your requirements? |
|
When using the |
|
Replacing the fixed length |
|
Dear maintainers, I implemented the solution suggested in #521 (comment) using the To test the implementation fairly, I artificially generated a CAN trace where each message was sent every 1 ms, then I compared both the The results show that our Moreover, I also generated a CAN trace with varying inter-arrival times between messages, so as to specifically measure the performance required to dynamically change the The delay between both the implementations (
|
Dear maintainers,
we are a research team at the University of Modena and Reggio Emilia, and we would like to contribute upstream our recent development on top of the
canplayertool.This pull request introduces a new extension that we dubbed RealCAN, which enables replaying an existing CAN
logfilerespecting the original relative timestamps. Our use case is to replay the given CAN trace in a more realistic way, so as to exercise our test network with the correct inter-arrival times that the ECUs expect.Changes:
-r, to enable real-time dispatching of CAN frames;We hope you'll find this contribution useful, and we are open to any suggestions on how to further refined this feature.
Wish you a great day!