Meaning of can.BusABC.state

[onvav]

Hello, please help me understand the meaning of class BusState(Enum):. I would like to show status of the interface (Kvaser Leaf and Vector VN1610 are on my desk) as specified by CAN specification (error active, error passive and bus-off state). But the @property def state(self): of the classes class KvaserBus(BusABC): and class VectorBus(BusABC): are not implemented.

I would like to implement them but I am not sure what it shall returns

• state of the CAN bus as defined by standard (active, passive, error=busoff)
• state of the Python class BusABC (active, passive=silent_mode, error=some_driver)

The documentation says that it returns "current state of the hardware". Does it mean state of driver connection with HW interface or state of CAN bus controller (active->passive->busoff)?

[christiansandberg]

Unfortunately it is a mix of both. I’m guessing the idea was that when reading it you would get the first and when writing to it you change the second. You could look at another implementation, like pcan for inspiration.

[bggardner]

I was writing a post to the message board, but found this issue and decided to post it here instead:

Could we consider renaming the BusState enum options according to the CAN specification (ERROR_ACTIVE, ERROR_PASSIVE, BUS_DOWN)?

Also, as I primarily use socketcan, there is a fourth state ("STOPPED"), which means the interface (not just the "bus") is down and will not automatically recover (as in Bus-Off) until brought back up. I'm not sure how applicable this is to the other interfaces, but maybe add this fourth state? Knowing the difference between Bus-Off and down/stopped in an application can be important. For those who are interested, the SocketCAN state can be found using "ip -d link show ", then finding the string on this line: "can state <STATE> restart-ms <TIME-MS>".

Additionally, defaulting BusABC's state property to be ACTIVE seems to be bad practice. It seems the better practice would be to either make the state property throw a NotImplementedError() by default or default to a new UNKNOWN enum value. If there is no way to detect the state for an interface and it actually needs the state property to return ACTIVE (and not UNKNOWN) or an exception, then only that interface's Bus class would override the state property. This may solve the "mix of both" issue by simply defining more states, or making them different class properties (hardware state vs. software state). I would prefer a single property (state) with more enum options.

I can work on a PR if pursuing this is worthwhile.

[bggardner]

For reference, from iproute2:

static const char *can_state_names[CAN_STATE_MAX] = {
	[CAN_STATE_ERROR_ACTIVE] = "ERROR-ACTIVE",
	[CAN_STATE_ERROR_WARNING] = "ERROR-WARNING",
	[CAN_STATE_ERROR_PASSIVE] = "ERROR-PASSIVE",
	[CAN_STATE_BUS_OFF] = "BUS-OFF",
	[CAN_STATE_STOPPED] = "STOPPED",
	[CAN_STATE_SLEEPING] = "SLEEPING"
};

[felixdivo]

Do we plan to agree on a naming for the 4.0 release? Would probably be a good idea to change it before and not after it.

[hardbyte]

Agree, needs a champion! My 2c is that BusABC.state should include all the options as @bggardner has suggested.

[bggardner]

PR #847 contains my suggestions, except for the default value of BusABC.state (see comment), which 4.0 would be a good time to change it. Let me know and I can update the PR to include it (probably the NotImplementedError() option).

[keelung-yang]

Why there is no a state to stand for RUNNING / NORMAL / BUS_ON ? If users want to check bus state whether in normal (without any error) by reading bus's state property, how should we write correct state getter? even if there is an device api can fetch status of CAN bus.

[bggardner]

The ERROR-ACTIVE state is equivalent to "normal". All three ERROR-* states mean the bus is "running", but in various states of error, and it could be argued that even BUS-OFF could be considered "running", as the bus can recover automatically from it without intervention. ERROR-ACTIVE, ERROR-PASSIVE and BUS-OFF are defined in ISO 11898-1. ERROR-WARNING is really just ERROR-ACTIVE when the errors haven't exceeded the ERROR-PASSIVE threshold (the warning threshold is implementation-specific). We are considering a STOPPED state so the application knows the CAN interface is disabled/inoperable as opposed to enabled/operable but in the BUS-OFF state.

[bggardner]

@hardbyte @felixdivo Now that 4.0 has been released and this was not incorporated :(, what's the plan? Maybe 4.1? I updated PR #847 with upstream so the change can be easily merged.

[felixdivo]

@bggardner thanks for reviving this. I'm not involved anymore, so @hardbyte maybe you have an opinion?

[zariiii9003]

I suggest adding these properties to BusABC

from enum import Enum, auto


class ErrorState(Enum):
    ERROR_ACTIVE = auto()
    ERROR_WARNING = auto()
    ERROR_PASSIVE = auto()
    BUS_OFF = auto()
    STOPPED = auto()
    SLEEPING = auto()


class BusABC(metaclass=ABCMeta):
    
    @property
    def error_state(self) -> ErrorState:
        raise NotImplementedError()
    
    @property
    def tx_error_count(self) -> int:
        raise NotImplementedError()
    
    @property
    def rx_error_count(self) -> int:
        raise NotImplementedError()

The BusState class should be deprecated and replaced by a silent: bool = False argument in BusABC.__init__() (and maybe a silent property).

[bggardner]

Point of order: The definition of bus state in ISO 11898-1 is "one of two complementary logical states: dominant or recessive"...equivalent to a zero or one. Elsewhere in ISO-11898-1, the three states error-active, error-passive, and bus-off are referred to as states of a node, not the bus (nor an "error state"). In the majority of this repo's use cases, no one probably cares about these definitions.

In any case, it is important to indicate these three "node states" to the application. The state of the can interface should also be indicated to the application, which would most likely be one of "up" or "down". However, if the interface is "up", the node will be in one of those three states; and if the interface is "down", the node will be in none of those states. I believe the iproute2 developers came to the same conclusion, which is why they have combined the "node states" with the "interface states" into the six "can states" above. I propose(d) we do the same.

So, props to @zariiii9003 for recommending error counts as well, but I think we can keep the BusState class, and document it is a combined indicator of both the CAN node state and the state of the CAN interface. I would be open to renaming it to CanState to be avoid nomenclature confusion with BusState.

[zariiii9003]

"Error state" is quite common in CAN interface docs. There's even an "error state indicator" bit. But i do not care about the name. Keeping BusState makes the transition more complicated.