Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New BusState members per #736 #1901

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

bggardner
Copy link

No description provided.

@bggardner
Copy link
Author

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

@zariiii9003
Copy link
Collaborator

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?

@bggardner
Copy link
Author

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).

@zariiii9003
Copy link
Collaborator

state is passed to subclasses of BusABC to enable monitoring mode like here:

state: BusState = BusState.ACTIVE,

It's ugly and should be replaced with listen_only (same name as in socketcan) in my opinion...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants