SHT Usermod: Fixed MQTT discovery using correct unit; Added getters and isEnabled() check

[ezcGman]

Hey there,

I'm sorry to open another PR so close after the big one got merged, but sadly a bug sneaked in :( Fixing it with these two commits!

Thanks and greetings,

Andy!

[SK360]

Tested the changes and temperature reports to HA properly now when set to F. Thanks for fixing!

[ezcGman]

Apologies for adding a few more changes to this PR. Usually it's bad practice to add different changes to a PR, but hence this is all around "my mod", I thought it's ok if I spare you from opening more PRs and just throw them in here.

I basically added a getter for the current temperature based on what's configured for the mod (F or C), a getter for humidity and a check if the mod is enabled.

I'm currently re-doing an older mod I did which also had SHT logic, which I'm now dropping in favor of this mode and avoiding duplicate code. Hence I added those getters and the enabled checker.

Hope it's OK that I added those changes here. Thanks!

[ezcGman]

Just realized I missed using flashstring helper in ::getUnitString. fixing...

[EDIT] Actually I'm not sure if that fixable. I don't think you can return a string using F(). Which return type would ::getUnitString need? __FlashStringHelper is abstract and I don't see it being specified somewhere...

Maybe I miss the knowledge on how that can be achieved :D

[blazoncek]

If you are using short strings and they appear several times in your source then it is ok to use classic strings without helper macros or PROGMEM keyword. This being due to the fact that classic string is stored in precious RAM eating away heap and there is a penalty using string from flash. All strings stored in RAM are optimised by compiler so that only one instance is stored where flash strings will appear as many times as they are entered in the code.

[ezcGman]

Sooooo, summarizing:

• Use PROGMEM / F() for strings where possible / as much as possible to save RAM
• But if you do and you have repeating strings, use PROGMEM and store them as class member variables as I do for the config strings?

[blazoncek]

Short string (up to 10-12 bytes), used many times (more than 2 across all source files) -> use regular string. Otherwise use PROGMEM / F() / PSTR()

[ezcGman]

Ok, think I'm getting it slowly :)

OK, but it generally doesn't hurt to put more into PROGMEM? Or does it? Because you said "there is a penalty using string from flash.": I guess you mean a performance penalty reading them? So actually don't put as much as you can? ;)

[ezcGman]

I think then there's nothing left to do for me. I do see it saying that there is one pending change requested, but that conversation is marked as resolved. So not sure if there is something left to do about your comment to that review:

There are some "not so good" code constructs throughout.

I could remove F() around strings that are "up to 10-12 bytes", if putting them into PROGMEM is actually an issue? Totally fine doing this, I'm just not 100% sure if that's requested, that's all :)

[blazoncek]

Will have another look on weekend and try to optimise if there is any need.

[ezcGman]

Sure, no rush! Would love to know where/how to optimize to do it right :) Excited! Thx!

[blazoncek]

@ezcGman please test it after my slight modification. If it still works it is good to merge. 😄

[ezcGman]

Thanks blaz! Confirmed everything still working. Also compared the two Fahrenheit values (what the lib returns (which calculates it from the raw read sensor value) plus now calculating it from the Celsius values) and they're exactly the same:

[SHT-Sensor] Fahrenheit calculated: 81.667198
[SHT-Sensor] Fahrenheit read from lib: 81.667198

So good to merge, thanks!