On 02/17/2011 12:05 AM, Kurt Van Dijck wrote:
> 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 :-)
I'm still not convinced. Imaging the user wants to look for state
changes. Then he will use CAN_ERR_CTRL bit in the CAN id. But he would
expect to see state changes to error active then as well.
>>>>> 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'.
What do you mean with hot path. It just adds a version dependency to the
error message. So it's always clear what formatting has been used.
>> 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.
I thin mantaining and incementing too similar stats is overkill. Also,
we have difficulties to map the CAN errors to the netdev errors:
http://lxr.linux.no/#linux+v2.6.37/include/linux/netdevice.h#L173
Wolfgang.
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
