search

hoylabs / OpenDTU-OnBattery

Software for ESP32 to talk to Hoymiles/TSUN/Solenso Inverters, VE.Direct devices, battery management systems, and related peripherals
GNU General Public License v2.0
301 stars 63 forks source link

fix: update mqtt subscriptions when topic was changed #1156

Closed AndreasBoehm closed 1 month ago

AndreasBoehm commented 2 months ago

Update mqtt subscription and update homeassistant config when topic was changed.

Fixes https://github.com/helgeerbe/OpenDTU-OnBattery/issues/1045

schlimmchen commented 2 months ago

Looks good, but it's a little too much to review on the phone. Is a review necessary? I tend to trust that this is good.

Are all OnBattery extensions covered with this?

AndreasBoehm commented 2 months ago

I went through all classes but because of all the back and forth i would not trust myself that i covered all OnBattery extensions with this.

Let's wait and get it fully reviewed.

schlimmchen commented 1 month ago

As far as I can tell, you covered all relevant subscriptions. However, I am not happy about duplicating each subscription string to implement the unsubscribing for two reasons: (1) occupies flash storage and (2) when adding new topics the module subscribes to, there is a high chance that the unsubscribe function is not taken care of as well.

What do you think about this idea: In the respective lambdas that call MqttSetting.subscribe() we know the full topic to subscribe to. After subscribing, we add the topic string to a container (I guess a vector). The unsubscribe() function iterates all these topics and calls unsubscribe for them. I know this uses more RAM, which is the only thing that makes me unsure that this is the right approach.

How about a frozen::map? That would live in flash, would be compact (no strings are defined two times), and we could iterate it to subscribe and to unsubscribe?

I just added a respective change. What do you think?

AndreasBoehm commented 1 month ago

I really like the idea, makes sense to do it that way 👍

github-actions[bot] commented 2 weeks ago

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new discussion or issue for related concerns.