re-factoring of HttpPowerMeter #594
Conversation
Added ability to deal with local host names (mDNS), remove use of FirebasedJson to save ~20kB build size, some changes to PowerLimiter to avoid setting new inverter power limits when not needed (=current limit as reported by inverter is within hysteresis)
|
Can we remove in |
|
Indeed, I cannot see FirebaseJson used anywhere else, so can be removed from platformio.ini. Done in last commit. |
There was a problem hiding this comment.
I appreciate that you switched the JSON parsing to a library that is already available, saving so much code. Good job! This PR is hard to do a review on because you changed a lot at once. The refactoring you did is desirable and a refactoring is always harder to review.
…tpPowerMeter For non IP address URLs, HttpPowerMeter now first tries DNS for resolution as done in WifiClient::connect, and only if that fails tries mDNS. For the latter to work mDNS needs to be enabled in settings. Log in console if mDNS is disabled. String building for Digest authorization still tries to avoid "+" for reasons outlined in https://cpp4arduino.com/2020/02/07/how-to-format-strings-without-the-string-class.html This should also be saver than just concatenating user input strings in preventing format string attacks. https://owasp.org/www-community/attacks/Format_string_attack
|
OK I made a new commit, hopefully addressing all your requests. Do you see the commits? Thanks a bunch for the review! |
OpenDTU console gets spammed with "WifiGeneric::hostByName() error when first trying to resolve the hostname via DNS. So reverse order: first try mDNS, if that fails try DNS. Also ensure that https bool is passed correctly to HTTPClient::begin(). Lastly, concatenate strings for building Digest authorization using "+" and not via snprintf.
|
@Fribur I'm sorry. With the latest merge of the upstream code, we have a merge conflict. Can you resolve it. |
|
Thanks for notifying me about the conflict...did not realize the ball is on my end to resolve. I just did. From my perspective this pull request is final. It is working reliably with those changes now since 2 days on my end. Please note however I cannot test the refactored digest authorization as I got no Digest compatible Power Meter (Shelly EM can only do basic authorization). So if you could verify Digest authorization after the merge that would be great! |
|
Thanks for resolving the conflicts. I know, I'm a bit lazy, but you know your code, better than me. I own, only a normal Sehelly EM, but I have a Shelly PM 1 plus (which support digest auth). I will give it a try. |
|
Just installed this PR locally.
But in the past I saw also connection problems to my shelly EM |
|
Exactly those errors (besides the mDNS issue) prompted me to begin looking into re-factoring HttpPowerMeter: see issue I opened prior to the re-ractoring. #579 Regretably this is not improved via this Pull Request. I came to realize this is only related to the frequency of querying the PowerMeter (I believe it is already limited to once per second in PowerLimiter class) and the configured timeout. When I set it to 2000ms via the WebInterface it works in 99% of the cases. Maybe this can be reduced by setting different timeouts for "Connect" and "Response"? What are reasonable values? Currently it is implemented like this: It might also have something to with the new threading/scheduling? Not quite sure which thread is responsible for the WIFI, the webinterface etc. Lastly: I also tried to query both phases of the Shelly EM with the exact same HTTP client with ONE connection (to reduce the connection refused lost issues): does not work, as Shelly EM forces "connection close" after each and every response. So each query needs to be done with a new http connection :-( |
|
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. |
Added ability to deal with local host names (mDNS), remove use of FirebasedJson to save ~20kB build size, some changes to PowerLimiter to avoid setting new inverter power limits when not needed (=current limit as reported by inverter is within hysteresis)