Set powerlimiter thresholds via MQTT

[LukasK13]

This pull request adds MQTT topics to set the thresholds of the dynamic power limiter via MQTT.
Closes #677

[schlimmchen]

This is a popular request and it is nice to see that you are willing to put time and effort into this!

However, please have a look at your changeset: You change the line endings of the two files you touched, and the whole file seems to have been swapped out because of this. Understanding what you've changed becomes very hard like this. Can you please revert the line endings on those files and force-push a new commit to your branch?

Once you did and once I had some sleep, I would like to give ideas on how to make this a polished feature.

[helgeerbe]

@LukasK13 can you do the changes @schlimmchen asked for. That makes life much easier for me.

[LukasK13]

Of course. Plus I will work on the autodiscovery in Home-Assistant in the next days. I think it will make sense to pull everything together.

[LukasK13]

I need some help finding the issue with the line endings. I'm developing on MacOS so LF is assigned to all files. What is the default line ending of the repository? Thanks.

[schlimmchen]

What is the default line ending of the repository?

Unfortunately, there is a mix. Most files are LF, but some are Windows-Style CRLF. The french locale is CRLF. Do you have the unix2dos utility on your Mac? You could use that to translate the file back to CRLF.

Plus I will work on the autodiscovery in Home-Assistant in the next days.

That is admirable, but I am not too happy about exposing all thresholds. Can we limit this to those that make sense to be changed dynamically? Especially the full solar passthrough settings might not even make sense for all setups, i.e., setups without a Victron charge controller. We should at least check if the VE.Direct interface is enabled or not before accepting MQTT thresholds controlling solar passthrough.

Another issue: You are changing the config values directly. They are not made persistent right away. However, once any setting is changed using the web app, they are made persistent. All of them. This is a very weird side-effect that is hard to explain/document, hard to understand, and hard to debug. I think that overridden values should go into their own variables, which are always volatile. And we desperately need a card in the live view that communicates to the user the current, actually active settings.

One more thing I remember from looking at the code last time: It would be best to categorize the thresholds in a new sub-topic "thresholds", like "powerlimiter/status/thresholds/voltage/start" instead of "powerlimiter/status/voltage_start_threshold".

Moreover, you need to catch exceptions that std::stoul or std::stouf throw. Otherwise the ESP will constantly reboot in case a value is published that cannot be parsed as an integer or float.

[LukasK13]

1. I played around and tried to fix the line ending issue. Even unix2dos didn't do the trick for me. Therefore I created a new branch form development and recomitted all changes on windows. Afterwards I merged the new branch into the old branch that is referenced in this pull request. This created some confusion in the history but at least fixed the diffs. Does it make sense to enforce a specific line encoding for the whole repository in the future?

2. This makes sense. I reduced the exposed command inputs to mode, start + stop SoC threshold and full solar passthrough (only if vedirect is enabled).

3. If this behavior is hard to explain / document I will have a hard time trying to fix this. I basically copied the logic to update the configuration from the corresponding web API. I guess the issue you mentioned also exists for the web API? At least my solution is consistent with the API behavior. How can I make this a permanent solution?

4. I tried to be consistent with the web API naming and the naming of all other components. However if you wish I change it to subtopics.

5. Done. I copied this part from MqttHandleInverter.cpp where the exact same problem is unhandled. This should be fixed as well.

[schlimmchen]

Thanks for putting in the effort, this already looks much better!

When using the web API, it is expected that the respective changes are persistet. Like if you were using the web app and submitted the form, you expect that the settings are persisted. However, when fiddling with MQTT topics, one does not expect that some device might be writing its flash memory each time the topic is published, even with the same value. I think this should be avoided.

To fix this, a variable for each override is needed. Those are then set by calls form the MQTT handler to the DPL, which manages the overrides. I guess it would be elegant to use std::optional, such that it can easily be determined whether or not a particular override is set. All these variables are more reasons to keep the list of thresholds which can be overidden, rather short. If you struggle with this, I can pick up from here. And again: The DPL needs its own card in the live view such that people know what settings are actually effective, especially when there are (more) overrides present.

I tried to be consistent with the web API naming and the naming of all other components.

Don't be. Those are internal names. Up until now, they were set by text fields in a form and each has a label, which possibly differs greatly from the internal variable name. You are now presenting those values over a new interface, and that should be well though out, as it will be a huge pain to change this in the future. I would appreciate this structure:

• powerlimiter/cmd/threshold/voltage/<start|stop|full_solar_passthrough_start|full_solar_passthrough_stop>
• powerlimiter/cmd/threshold/soc/<start|stop|full_solar_passthrough>
• powerlimiter/status/threshold/voltage/...
• powerlimiter/status/threshold/soc/...

What do you think?

Oh boy, that yield() must go! Where did you copy that from?

And to be honest, I think the way the MQTT handler function is used it not very clever. I know you copied this, but that does not make it right. You bind the handler to all different topics the same way, and then you need to first find out for which topic the handler was called. That's... Well, that's dumb. Instead, add another parameter to the function, which takes an enum value, and bind that enum to the respective value for each topic that you subscribe. Then you just look at the enum value when the handler is executed and you know immediately for which topic a value was received. Look at MqttHandleHuawei.cpp, where Malte did a good job.

[LukasK13]

Well, that's dumb.

I'm sorry for my bad code quality. This is actually my first time working with C++. I think this is a really cool and very important project and, just like you, I want to contribute to make this awesome project even better. There is no need to become personal or offensive.

1. Thanks for the clarification, I understand your point and this definitely needs to be fixed. Alternatively to your proposed solution it might be enough to just compare the new value received via MQTT with the current value in the configuration and only update the configuration when these two values differ. What do you think about that? If you prefer your solution I fear I might not be able to implement that with my limited C++ knowledge. So this will be up to you then.

2. I'm fine with both solutions. I will try to find the time to implement your proposed topic structure.

3. The yield() wasn't introduced by me. It's already present in the current development branch

4. You are right, this is definitely not the best implementation. Having this implementation copied from another class doesn't make it right but it doesn't make this wrong either since I used the "coding standards" of this repo. I will try to change this to Malte's implementation in the next days.

[schlimmchen]

There is no need to become personal or offensive.

Yeah, I tend to be drastic and/or offensice, sorry. Thanks for dealing with it. I continue to remind myself to be respectful. In this case let me clarify that the assessment, in my mind, was not addressed to you at all since I thought you copied that portion from elsewhere in the project, didn't you? I don't blame you for that. We can still do better 💪

I will try to find the time to implement your proposed topic structure.

That would be great! As I mentioned, it's important to do it right the first time, as changig it in the future becomes very hard (breaking change). Since you would need to extend the tokenization approach then (more to tokenize), you may want to look at Malte's implementaiton binding the callback method to an enum so you don't have to do the tokenization at all 😉

Alternatively to your proposed solution it might be enough to just compare the new value received via MQTT with the current value in the configuration and only update the configuration when these two values differ. What do you think about that?

I thinks that's a very effective mitigation approach 👍

Here is some pseudo-code, maybe you can get ideas from it?

enum class Subject: unsigned {
  voltageStartThreshold,
  voltageStopThreshold,
  ...
};

void mqttCallback(Subject s, [MQTT callback signative]) {
  float val = float_from_string_with_try_catch(...);
  // return if conversion from string to float failed

  switch (s) {
    case voltageStartThreshold:
      if (config.startThreshold == val) { return; }
      config.startThreshold = val;
      break;
    case ...
  }

  // not reached if the value did not change
  config.write();
}

[LukasK13]

Thanks for your ideas and thoughts.
I implemented and tested all suggestions👍
Besides, I support the creation of a card for the power limiter in the live view. Unfortunately my JavaScript is even worse than my C++, so I might not be helpful here.

[schlimmchen]

Very nice!

One more thing: We should also return in case MqttPowerLimiterCommand::Mode:, so we do not write the config when changing the DPL mode.

And a minor detail: PowerLimiter MQTT handler: cannot parse payload of topic '%s' as int: should be .... as floating point value.

I cannot really comment on the Home Assistent integration, but it looks good. That's only to display settings, not to change them, right? And you decided to only integrate the SoC-based thresholds? Maybe we can guard them such that they are not published to Home Assistent if the battery interface is not enabled? I guess that would be good to avoid confusion.

Besides, I support the creation of a card for the power limiter in the live view.

One of the many ideas I would love to implement... I really wich I had even more time to spend 😊

[LukasK13]

Done :-)

[schlimmchen]

@LukasK13 I allowed myself to fix the logic in the home assistant handler. What do you think must be addressed or tested before this change is mature? I plan testing it this evening.

[Napizaki]

Hello, I downloaded the latest build and tested it --> it works!
I personally have one more wish: would it be possible to take the "Upper power limit" into consideration?
Thx

[schlimmchen]

Please open a feature request. Let's do this in another changeset and get thid PR merged.

And thanks for testing. I will test this as well and merge soon. I don't think that @LukasK13 would mind?

[LukasK13]

Sorry for my late response, had some busy days.
@schlimmchen thank you for your fixes, this looks good to me. I hope to find this weekend the time to test all changes. However your changes are only code improvements so I don't expect anything to break. My version before your changes is already tested and in productive use ;-)

[schlimmchen]

I had some trouble rebasing and squashing this, mainly due to the line-endings nightmares... I will test the Home Assistent integration for sure, as you said that you cannot test it since you don't have a Home Assistent installation. We'll see tomorrow.

[LukasK13]

This seems to be a misunderstanding, of course I do have a Home-Assistant installation and I tested the integration.

[LukasK13]

Alright, I synced my branch with the development branch. After compiling and uploading to my ESP32, everything seems to be working perfectly. I tested setting the DPL Limit with Home-Assistant and the autodiscovery. In my opinion this is ready to be merged. What do you think @schlimmchen ?

[schlimmchen]

I am in the process of testing and I like it. 💪 There are still minor issues, and I added more logic. Stand by for an update later this evening. I am willing to merge this today.

[schlimmchen]

This seems to be a misunderstanding

Yup, sorry about that.

I noticed that, strictly speaking, we shall not write the config struct from within the MQTT callback, as it runs in a different context than other tasks that might alter it as well (the Web app in particular). However... I guess we can try our luck with this one.

I corrected the following:

• V shall be the unit when printing voltage thresholds, not % (copy'n'paste error).
• The first version of the onCmdThreshold method was still present in MqttHandlePowerLimiterClass, so I removed it.
• removed one occurrence of trailing whitespace in MqttHandlePowerLimiterHassClass (this is my fault).

The changes were squashed and rebased onto the current development branch.

I then added another check in the Home Assistent integration: Do not announce thresholds at all if the inverter is solar-powered (a new option, see #733). And for completeness, the voltage thresholds are also made auto-discoverable. @LukasK13 did you have a particular reason why you did not add those? What happened in 5c66234?

SoC thresholds are not published to the broker at all unless the battery interface is enabled and SoC values are not set to be ignored.

The sensors are set to expire if Home Assistent Expiration is enabled. This is very useful when the config changes, so newly irrelevant values (when settings) are eventually removed from Home Assistent.

Force Home Assistent update when relevant settings change, i.e., making new or other thresholds auto-discoverable depending on settings.

[schlimmchen]

More minor issues in #737.

[LukasK13]

Thank you for your careful review and the fixes, great work.

I removed the voltage thresholds after this comment of yours. 'That is admirable, but I am not too happy about exposing all thresholds. Can we limit this to those that make sense to be changed dynamically?' Maybe I missunderstodd that one.

Finally thanks for merging :-)

[github-actions]

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.