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.
