victronenergy / venus

Victron Energy Unix/Linux OS
https://github.com/victronenergy/venus/wiki
580 stars 73 forks source link

mqtt-rpc: disabling all canbusses results in vup-9 error #366

Closed martinbosma closed 5 years ago

martinbosma commented 5 years ago

image

There is already some code in mqtt-rpc to find the right canbus:

    dbusSession = dbus.SessionBus() if 'DBUS_SESSION_BUS_ADDRESS' in os.environ else dbus.SystemBus()
    services = dbusSession.list_names()
    can_services = [ x for x in services if "com.victronenergy.vecan.can" in x ]

    if len(can_services) != 1:
        raise WrongNumberOfCanInterfacesDetected()

    can_interface = can_services[0].split(".")[-1]

    return "socketcan:{}".format(can_interface)

(https://github.com/victronenergy/mqtt-rpc/blob/master/mqtt-rpc.py#L76)

Logical fix seems to be to make it not call vup in case there is no canbus. As expected, there is no vecan dbus service on D-Bus for that system:

# dbus -y
org.freedesktop.DBus
com.victronenergy.solarcharger.ttyO2
com.victronenergy.battery.ttyO0
com.victronenergy.system
com.victronenergy.hub4
com.victronenergy.qwacs
com.victronenergy.logger
com.victronenergy.fronius
com.victronenergy.vebus.ttyO1
com.victronenergy.grid.cgwacs_ttyUSB0_di30_mb1
com.victronenergy.settings
net.connman
fi.epitest.hostap.WPASupplicant
com.victronenergy.pvinverter.cgwacs_ttyUSB0_di32_mb2
fi.w1.wpa_supplicant1
mpvader commented 5 years ago

@izak Why didn’t we see an error in the logs when testing with len() != 1 ?

izak commented 5 years ago

From what I can see, the original author of this code did not consider a lack of a canbus any reason to give up hope or raise an error. If there isn't one and only one canbus, the code simply calls vup without a --canbus argument.

Of course vup then tries to find a canbus on its own, fails, and returns with exitcode 9.

I can fix it with this code (in cases where there is no canbus), but I think it can be made simpler.

        responder = OpResponseSender("vuplist", parameters, errorprefix="vup-")
        responder.send({"status": "running",
            "xmloutput": '<vuprun><discovered></discovered></vuprun>'})
        responder.send(
            {"status": "finished", "exitcode": 0}, finished=True)

This fakes an empty result, and then declares that it is finished. It would be better if we can skip the fake empty result and just go straight to finished, or in some other way signal non-support. Going straight to finished without sending the empty result does not presently work.

VRM cannot move on without the intermediary empty result.

mauritsvdvijgh commented 5 years ago

How did you test that VRM "cannot move on" without intermediary empty result? Curious about the logs for that one. If it is valid XML you are sending it will find all the device nodes in there and return the devices, just wondering where this goes wrong? (Maybe with empty xml?).

mpvader commented 5 years ago

The VRM side of mqtt-rpc was modified to make it cope with an empty xml response:

https://github.com/victronenergy/vrm-mqtt-rpc/commit/871d068c4bc617b7092777d35401ab847695ccc8