On Wed, Feb 16, 2011 at 09:31:30PM +0100, Oliver Hartkopp wrote:
> On 16.02.2011 21:04, Wolfgang Grandegger wrote:
> > On 02/16/2011 08:00 PM, Oliver Hartkopp wrote:
> >> On 16.02.2011 09:10, Wolfgang Grandegger wrote:
>
>
> >>> But wrong termination may result in other errors as well and there I
> >>> think it could not be used reliably for the purpose you describe above.
> >>
> >> Ok. But i think we could leave it as-is. I don't think it's worth to break
> >> the
> >> definition here.
> >
> > I prefer the clean solution. I don't think it's worth to keep it in the
> > CAN id. It will just confuse users.
>
> Is also OK for me.
>
> Would you suggest to reuse the bit-value from CAN_ERR_ACK (0x0020U) for
> CAN_ERR_ACTIVE then? I've done this in my suggested patch below.
>
> >> For that reason i would prefer to have the CAN_ERR_ACTIVE bit *only* in the
> >> CAN-ID status bits and not in the detailed data[1] (CAN_ERR_CTRL_ACTIVE) as
> >> you and Kurt suggested.
> >
> > OK, but then I would drop CAN_ERR_CTRL_ACTIVE.
>
> Yes, thanks. That was my intention.
After thinking and thinking, I see the logic for both solutions, so
this too is fine :-)
>
> >>> We then could or even should add a version information into the error
> >>> message. I think data[5] could be used for that purpose.
> >>
> >> It's like having the CAN core version information where Dave was
> >> complaining
> >> when it gone Mainline. Who should check version numbers and maintain all
> >> this?
> >
> > This would allow users already using the error flags to easily migrate
> > to the new and changed error types. My idea here is:
> >
> > cf->data[5] = CAN_ERR_VERSION;
> >
> > Also candump could use it to avoid displaying rubbish with old kernels.
IMO it's not so good put the version in a 'hot path'.
>
> Hm - maybe then it's better to retrieve the Kernel version with uname(2) ...
>
> > I started writing the patch and realized that we need some more
> > clarification and cleanup. Next point is how we increment the
> > stats->[r|t]x_error counters? It's not done consistently and it's not
> > even possible to assign the direction of bus errors properly. Have a
> > look to the drivers to realizes the mess. Should we do that at all? We
> > have our own stats of CAN errors:
> >
> > struct can_device_stats {
> > __u32 bus_error; /* Bus errors */
> > __u32 error_warning; /* Changes to error warning state */
> > __u32 error_passive; /* Changes to error passive state */
> > __u32 bus_off; /* Changes to bus off state */
> > __u32 arbitration_lost; /* Arbitration lost errors */
> > __u32 restarts; /* CAN controller re-starts */
> > };
> >
> > I tend to drop touching the network stats completely. What do you and
> > others think.
>
> Using the common network stats would have the advantage, that you can easily
> see problems with standard tools like 'ifconfig' or 'cat /proc/net/dev'
After dropping network stats, some other stats mechanism will appear with
even less defined semantics, since some kind of stats are usefull (we even
have some extra stats for CAN errors!).
It is a bit messy now, I agree.
Kurt
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
