mysensors/NodeManager

[Question about a PR] HomeAssistant controller compatibilty

nekromant opened this issue · 10 comments

nekromant commented

Hello and thanks for the awesome project. I've been hacking NodeManager to run nicely with HomeAssistant, but ran into a few problems that required altering the some of the core API and concepts of the NodeManager. So far I've fixed SensorDimmer and SensorNeopixel to play nicely with hass. Before opening a pull request I wanted to ask if these changes are okay, and what branch I should file them against. Here's the stuff I've put together so far:

  • Sensors are made in a way that children always have a unique child id. Currently SensorDimmer exports 2 children for each dimmer. One with V_LEVEL, one with V_STATUS. HomeAssistant treats them as separate entities and expects a single child to accept and report both V_PERCENTAGE and V_STATUS for the light to show up.

To implement this I added a

Child* Sensor::getChild(uint8_t child_id, uint8_t ctype);

method to Sensor.cpp and made some changes so that it is called in a few parts of NodeManager to make use of the new variant of call. This allowed a sensor to have children with same IDs.

  • HomeAssistant requires nodes to report their initial states to the controller before they can show up. For this to play nicely I added a special hack that reports all the states of the switches of the nodes at the very start. It is done on the very first iteration of the loop. Ideally, I want to make an option, so that the node requests initial states from the controller, and after receiving whatever states are available, reports everything once to the gateway.

Other minor fixes:

  • SensorDimmer does dimming synchronous, with wait(). Receiving a few dimmer commands during actual fading results in a node crash, I've changed that for async operation.
  • I've implemented a SensorParent sensor that exposes the node's current parent node. Having this sensors allows building nice topology charts (See one attached below)
    chart

Well, that's all.

castorinop commented

@nekromant Can you submit a PR ?

nekromant commented

@castorinop Against development or master branch? Let me know. Will rebase and submit ASAP.

castorinop commented

@nekromant i have same issues with home assistant.

nekromant commented

@castorinop I've put up my fork over to github here: https://github.com/nekromant/NodeManager

Please note, it's all still work in progress: I haven't finalized SensorDimmer that needs async handling of fading (or it will crash the node), haven't yet ported my SensorChild and my Node UpLink quality checker and would really love to hear back from NodeManager maintainers if they're interested in accepting these changes.

castorinop commented

@nekromant thanks! should be use simpleTimer for async events

nekromant commented

also a queue implementation https://gist.github.com/castorinop/90f1909376bf1adca896ac425e168f2a

I don't think it's worth queuing packets, since the nodes have VERY little ram. Timers for Dimmer are on my TODO, but I haven't finished it yet.

user2684 commented

@nekromant thanks for bring this up. I was aware HA requires a specific behaviour in order to work correctly and this could be a good excuse to make NodeManager fully compatible with it, finally. I remember last time I get into the code for something similar to this, having the same child_id responding with multiple types was kind of a challenge and breaking other things here and there but let's dig into it more seriously now.

The only constraint I'd like to put in place for whoever is willing to contribute on this, is to ensure the original behaviour of NodeManager is not changed so to avoid messing up with existing users but instead make everything optional wherever possible. If this would bring an impact on the code size, we can make it as a feature which can be enabled/disabled like other already available in NodeManager.

But first we need something working, I agree. A PR against the development branch is the way to go.
Thanks!

  • 👍2
nekromant commented

@user2684 Thanks a lot for the answer, I'll submit the first PR against the core in a few days. I don't have too much time, so I'll send in code in small batches. The nasty stuff about current HomeAssistant implementation is that actual sensors' code in NM need changes, but I'll think how to implement it as compatible as possible.

**Submitted in #533 (core) **

  • Implement some core changes to NodeManager that will allow a ChildID accept several message types
  • Make NodeManager spit out initial sensor values on boot (or they won't appear in HomeAssistant)

** WIP (core) **

  • Request initial values from controller on boot (before spitting out inital values, e.g. restore brightness levels for dimmers)
  • Allow NodeManager optionally send out heartbeats for always powered nodes. This is useful to track spurious reboots and can be later used to implement the availability feature like zigbee2mqtt

** Planned **

  • Fix SensorDimmer to use new API
  • Make SensorDimmer asyncronously process fading, otherwise a message or two while fading crash the node
  • Port SensorParent from my non-nodemanager project
  • Fix SensorNeoPixel to support HomeAssistant (Or, prehaps, completely re-implement it using FastLED)
  • 👍2
  • 🎉1
nekromant commented

Here goes the first one. I have updated the TODO and will edit later the comment once more stuff is ready.