python-can icon indicating copy to clipboard operation
python-can copied to clipboard

New BusState members per #736

Open bggardner opened this issue 1 year ago • 4 comments

bggardner avatar Dec 01 '24 22:12 bggardner

I also have a change ready that queries the system for the state of a SocketcanBus, but it depends on the pyroute2 package. I didn't want to cause another dependency, but this seemed to be the most efficient way to query the interface state. Should it be a package-wide dependency, or lazily loaded within the class/method?

    @property
    def state(self) -> BusState:
        try:
            with pyroute2.IPRoute() as ipr:
                operstate = ipr.get_links(ipr.link_lookup(ifname=self.channel)[0])[0].get_attr('IFLA_OPERSTATE')
        except:
            return BusState.UNKNOWN # Could be BusState.STOPPED instead (channel doesn't exist)
        if operstate == 'ERROR-ACTIVE':
            return BusState.ERROR_ACTIVE
        elif operstate == 'ERROR-PASSIVE':
            return BusState.ERROR_PASSIVE
        elif operstate == 'BUS-OFF':
            return BusState.BUS_OFF
        elif operstate == 'STOPPED':
            return BusState.STOPPED
        else:
            return BusState.UNKNOWN

bggardner avatar Dec 02 '24 14:12 bggardner

Generally i agree with this, but the consequence is, that BusState should not be passed to BusABC.__init__() anymore. That could be replaced with a universal parameter listen_only: bool for all interfaces. The BusABC.state.setter should also be removed and replaced with a BusABC.listen_only property,

It's a breaking change, but we could do this in a 5.0 release. We have to cleanup all the deprecations at some point anyway. @hardbyte What do you think?

zariiii9003 avatar Dec 04 '24 21:12 zariiii9003

I'm not sure I follow you since BusState isn't passed to BusABC.__init__(). I agree that BusABC.state.setter should be removed, though I think listen_only: bool is not a direct replacement, but a "feature" of the specific interface. To prevent a breaking change, we could just add the BUS_OFF and UNKNOWN members, then replace them with the more appropriate names in 5.0 (ACTIVE => ERROR_ACTIVE, PASSIVE => ERROR_PASSIVE, ERROR => STOPPED).

bggardner avatar Dec 04 '24 21:12 bggardner

state is passed to subclasses of BusABC to enable monitoring mode like here: https://github.com/hardbyte/python-can/blob/654a02ae24bfc50bf1bb1fad7aab4aa88763d302/can/interfaces/pcan/pcan.py#L124 It's ugly and should be replaced with listen_only (same name as in socketcan) in my opinion...

zariiii9003 avatar Dec 04 '24 22:12 zariiii9003