Thanks guys, I'll get back to you after addressing your concerns.
Oliver,
Right, I've been using "-n 6:15:0" to count from 0 to 15 in our application. Apologies for the email footer, but it is automatically inserted by our email server I'm afraid.
I have some additional enhancements to support more types of checksum calculations too, but I'll wait until we work through this counter feature before trying to submit those.
Brad
TESTING THE FUTURE for OVER 20 YEARS
NOTICE: This e-mail and any attachments may contain confidential, privileged and proprietary information of D & V Electronics Ltd. and all such material is subject to copyright protection in favor of D & V Electronics Ltd. Any unauthorized disclosure or other use of this e-mail or the information contained herein or in any documents attached hereto may be unlawful and is strictly prohibited. If you have received this e-mail in error, please notify the sender immediately and delete this e-mail without reading, printing, copying or forwarding it to anyone. This email was sent from D & V Electronics Ltd. To receive no further email communications, you can send an email to unsubscribe@xxxxxxxxxxxxxxxxx. D & V Electronics is located at 130 Zenway Blvd. Woodbridge, Ontario, Canada.
-----Original Message-----
From: Oliver Hartkopp <socketcan@xxxxxxxxxxxx>
Sent: August 29, 2018 2:04 PM
To: Brad Drehmer <b.drehmer@xxxxxxxxxxxxxxxxx>; Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx>
Cc: linux-can@xxxxxxxxxxxxxxx
Subject: Re: [PATCH can-gw] add support for counter byte
Hi Brad, hi Marc,
first: I think this functionality is cool :-)
The review from Marc covered already most of my potential comments too.
On 08/29/2018 07:30 PM, Brad Drehmer wrote:
>
> The index of the counter byte is used inside the cgw_count function in the line:
> cf->data[msgcounter->result_idx] = count;
>
> To implement this feature I followed the lead of some of the existing features, so it should work in a similar way. cgw_chk_result_idx_parm is pretty much the same as cgw_chk_csum_parms, except instead of checking the validity of 3 indexes (from, to, and result), it checks only one (the result index where the counter goes).
Marc is right. Your relative index idea does not work.
If msgcounter->result_idx is negative, you need to do something like:
int idx;
if (msgcounter->result_idx >= 0)
idx = msgcounter->result_idx;
else
idx = cf->can_dlc + msgcounter->result_idx;
if (idx >= 0)
cf->data[idx] = count;
Got that?
Additionally the comment/desciption, at which value of the max_count the rollover happens is unclear.
E.g. to count from 0 .. 15
.value has to be 0 and .max_count has to be 15?
.value can be not 0, right?
What happens if you start with .value = 234 and .max_count = 5 ??
If .value is exposed to the API the counter starts there (but only the first time?!?).
Finally I would suggest to add a .mask element, which can contain a bitmask for the byte where the index is stored in.
E.g. if you want to have a rolling counter 0 .. 15 in data[0] and you would like to prevent the bits 4..7 in that data[0] you would be able to provide a mask of 0x0F to only modify the counter.
Or do you thing we can calculate the mask automatically when the counters max_value are only allowed as 2^n-1 ?
Best regards,
Oliver
ps. You have a pretty long footer. This is not common on mailing lists.
