On 02/18/2011 04:45 PM, Oliver Hartkopp wrote:
> On 17.02.2011 20:17, Wolfgang Grandegger wrote:
>> 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:
>
>
>>>>>> 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.
>
> State changes are only located in the CAN-ID.
>
> If you look into
>
> http://lxr.linux.no/#linux+v2.6.37/drivers/net/can/flexcan.c#L404
>
> /* process state changes depending on the new state */
> switch (new_state) {
> case CAN_STATE_ERROR_ACTIVE:
> dev_dbg(dev->dev.parent, "Error Active\n");
> cf->can_id |= CAN_ERR_PROT;
> cf->data[2] = CAN_ERR_PROT_ACTIVE;
> break;
> case CAN_STATE_BUS_OFF:
> cf->can_id |= CAN_ERR_BUSOFF;
> can_bus_off(dev);
> break;
> default:
> break;
> }
>
> you can see the problem.
>
> /* process state changes depending on the new state */
> switch (new_state) {
> case CAN_STATE_ERROR_ACTIVE:
> dev_dbg(dev->dev.parent, "Error Active\n");
> cf->can_id |= CAN_ERR_ACTIVE;
> break;
> case CAN_STATE_BUS_OFF:
> cf->can_id |= CAN_ERR_BUSOFF;
> can_bus_off(dev);
> break;
> default:
> break;
> }
>
> would just be more logical than stating some 'protocol error' that indicates
> in fact 'no error' ...
I think we already agreed that cf->data[2] = CAN_ERR_PROT_ACTIVE is
wrong. I'm not speaking about that. State changes (CAN_ERR_CRTLs) are in
data[1]:
data[1] |= CAN_ERR_CRTL_TX_PASSIVE;
data[1] |= CAN_ERR_CRTL_RX_PASSIVE;
data[1] |= CAN_ERR_CRTL_TX_WARNING;
data[1] |= CAN_ERR_CRTL_RX_WARNING;
A argue(d) that
data[1] |= CAN_ERR_CRTL_ACTIVE; (*)
is more intuitive than
can_id |= CAN_ERR_ACTIVE; (**)
The user would expect the information of the new state in data[1] as
well. But well, we aready have CAN_ERR_BUSOFF in the can_id, for good
reasons, and as the ACTIVE event could also be used for bus off
recovery, ...
Wolfgang.
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
