victronenergy / node-red-contrib-victron

MIT License
90 stars 18 forks source link

Show CustomName in edit options #6

Closed pkronstrom closed 5 years ago

pkronstrom commented 5 years ago

For example, show Battery 'House' - Voltage (V) if a CustomName is set for the service, instead of the current Battery - ttyO0 - Voltage (V)

On the Backend-side, the VictronClient will need to make a separate request to /CustomName (or /ProductName) and register a handler for the incoming data. This has to be saved to the cache in order to show it on the Node-RED UI. I need to think of a good way to do this, so that it works on other devices, too.


Taken from a Slack discussion:

instead of the port, show the (custom) name of the device: use the text under /CustomName when available. And if not; then use /ProductName. The port (ie ttyO2), which is shown now, doesn't mean anything to installers. Herewith some examples:


root@beaglebone:~# dbus -y com.victronenergy.solarcharger.ttyO2 /ProductName GetValue
'BlueSolar Charger MPPT 150/35 rev1'
root@beaglebone:~# dbus -y com.victronenergy.solarcharger.ttyO2 /CustomName GetValue
Traceback (most recent call last):
  File "/usr/bin/dbus", line 364, in <module>
    ret = obj.object.get_dbus_method(method.name, iface.name)(*args)
AttributeError: 'NoneType' object has no attribute 'name'

and a BMV, that does have a custom name:

root@beaglebone:~# dbus -y com.victronenergy.battery.ttyO0 /ProductName GetValue
'BMV-702'
root@beaglebone:~# dbus -y com.victronenergy.battery.ttyO0 /CustomName GetValue
'Hydraulic battery'
pkronstrom commented 5 years ago

Current service & object-path discovery relies on dbus-listener.js polling the dbus root for available ~services~ paths. However, for e.g. battery monitors, this does not work properly, so the /ProductName is not visible to Node-RED.

See https://github.com/sbender9/signalk-venus-plugin/blob/master/dbus-listener.js#L102

Currently, I see a few options for this a) fix the root polling somehow, so that all services support requesting the root path b) implement a getValue function for dbus-listener.js to request specific values and initialize it on this 'device discovery' phase

Currently, the latter seems more plausible

mpvader commented 5 years ago

Hey @pkronstrom; something must be wrong there: all services you care about support a GetValue() or GetText() method call on the root path (/).

Try on a real system. Perhaps the issue is that the dbus-recorder doesn't support this? Although I think that was also fixed. Not sure though.

mpvader commented 5 years ago

And you wrote:

Current service & object-path discovery relies on dbus-listener.js polling the dbus root for available services.

Here you mean paths I think, not services? Or?

pkronstrom commented 5 years ago

Paths, indeed

pkronstrom commented 5 years ago

Getting the ProductNames from battery services work quite well from the command line dbus -y com.victronenergy.battery.ttyO0 /ProductName GetValue -> 'BMV-702'

so I implemented a quick getValue function (the a-option) for dbus-listener.js to specifically request the value.

I think I can just separately request CustomNames / ProductNames for desired services using this. So, the default way of dbus-listener polling the root path isn't really necessary here (the a-option). It would've made it a tiny bit easier, though.

mpvader commented 5 years ago

Hi, I wrote this:

Try on a real system. Perhaps the issue is that the dbus-recorder doesn't support this? Although I think that was also fixed. Not sure though.

Did you do that? In your reply your not coming back to that.

Or, if your workaround is real simple: then fine too. But then say that.

pkronstrom commented 5 years ago

Hi, I wrote this:

Try on a real system. Perhaps the issue is that the dbus-recorder doesn't support this? Although I think that was also fixed. Not sure though.

Did you do that? In your reply your not coming back to that.

Or, if your workaround is real simple: then fine too. But then say that.

I totally missed the mention on rootpath, sorry! I will try it on a real system when possible. The current workaround works well with venus-docker, and allows me to continue with the development, so I will leave it as is.

mpvader commented 5 years ago

ok good; lets keep the work around on your end. I'll discuss the rest with Izak. For the future and now that we have the venus-docker container its good to get that as close to reality as possible; taking away the complexity.

So this issue can be closed now? Up to you.

pkronstrom commented 5 years ago

ok good; lets keep the work around on your end. I'll discuss the rest with Izak. For the future and now that we have the venus-docker container its good to get that as close to reality as possible; taking away the complexity.

So this issue can be closed now? Up to you.

I will close this for now. I will test it on a real system when we get it running. Agreed on keeping things simple.