mysensors / NodeManager

Plugin for a rapid development of battery-powered sensors
130 stars 82 forks source link

HomeAssistant Compatibility: Core Changes #533

Open nekromant opened 3 years ago

nekromant commented 3 years ago

The pull request implements the basic changes to NodeManager's code required for HomeAssistant compatibilty as discussed in #532

@user2684 You may want to have a detailed look at my changes. They seem trivial, but since I'm not very familiar with the codebase I might've left a few goblins lurking here and there.

user2684 commented 3 years ago

Excellent thanks! Let me review the chances during the weekend and I'll let you know. Of course I will wait for a first full implementation before merging it into the development branch. Meanwhile as you may have noticed, I moved some of the automatic tests here into Github so at every commit, most of the sensors are automatically compiled and if something goes wrong a red cross is showing up like in this case (which is ok since it is WIP). You can then review the output to see what has failed. This is still not 100% accurate but could help in finding potential compatibility issues with the library ecosystem. Thanks!

user2684 commented 3 years ago

Hi, I see a bunch of limitations to this approach which makes me think we should tackle the problem from a different angle, which is getting deep into the atomic data structure and change them radically if we want to adapt this The biggest one is Children are referenced by index in their underlying data structure so if you register a child with the same id, the previous one will be lost, regardless of checking for the type as well. We probably need to think about an index which is unrelated to the child id but this requires an additional sort of conversation table. This could require some good rework but let's find the best approach first and then we will think about the implementation.

Other more general guidelines, don't call child->sendValue() directly, but we need to find the way to go through sensor->loop() otherwise we lose PowerManager ad other stuff implemented there.

I'm not a HA user but I also remember reading somewhere when you send a value for a sensor you need also to send the same empty just after, is that true? Thanks

nekromant commented 3 years ago

The biggest one is Children are referenced by index in their underlying data structure so if you register a child with the same id, the previous one will be lost, regardless of checking for the type as well.

Hmm, I didn't notice that, guess I have to take another good dive in the code. Reworked dimmer code seemed to play nicely with those changes, so I assumed that it was working.

I'm not a HA user but I also remember reading somewhere when you send a value for a sensor you need also to send the same empty just after, is that true? Thanks

The current implementation just needs an initial value for an instance to show up. Nothing else. It will save it in a json file to persist across restarts, but since the file frequently overriten I keep it in tmpfs on my instance.

I can post my quick and dirty nodemanager replacement I use now for my nodes if that can serve of any reference.

On a side note. As far as I know, one of the core hass developers @MatinHjelmare is working on a new mysensors integration for HomeAssistant that will be based off a new library: https://github.com/MartinHjelmare/aiomysensors

Perhaps we can just cooperate to make the new integration and library as painless as possible for all of us?

nekromant commented 3 years ago

Other more general guidelines, don't call child->sendValue() directly, but we need to find the way to go through sensor->loop() otherwise we lose PowerManager ad other stuff implemented there.

Another idea here is an FSM, that will have a few states for each child:

As for the last state, my most recent experiments with a moderate (15 devices) network show that reliability becomes a problem for actuators once node is a hop or two away from the controller and guaranteed delivery is a must for actuators. Perhaps handling that would be best in the same FSM, but I'm still thinking about it.

user2684 commented 3 years ago

I see your point and appreciated it, yeah we need to think more strategically and potentially re-think the way NodeManager handles its sensors and children. I like the FSM idea. Sending out the initial value is not a big issue (actually this is done already when a reporting interval is set), the multiple types associated to the same ID is instead a bit more tricky. As soon as I will have some free cycles I'll pick my brain on it, definitely requires also for me a good deep dive into the code to remember how the different data structures are linked with each other. Thanks!