Adjust temperature under usb power

[macifell]

The problem:

When running on USB power the enviro indoor reports temperatures that are higher than the true values. This can easily be observed by connecting a battery pack and plugging and unplugging a usb power source, allowing several readings to be taken in each state.

https://github.com/pimoroni/enviro/issues/137 https://github.com/pimoroni/enviro/issues/134

The solution:

This fix allows configuration settings to be used to determine when and by how much to reduce the temperature. It also adjusts the relative humidity as this is dependent on the temperature. Although there are two new config settings, only one is intended to be modified manually - the usb_power_temperature_offset, since I'm not sure how much it will vary between models and under different circumstances.

Some code was added to automatically detect usb vs battery power and update the usb_power config setting, though it may take one reset iteration for the correct value to be used. Writing to the config file multiple times is not ideal, but is a rudimentary solution to persisting state between runs. There might be a better way to easily determine if we're on battery power vs. USB.

I only have access to an enviro indoor at the moment, so I did not attempt to fix the other models - though the same code can theoretically be used.

[macifell]

I'm not sure what the policy is for adding options to the config file. These changes would either require that a user runs the setup process again or manually adds entries to the config file. A method for detecting and adding missing config settings could be added if it seems like that would be helpful, but for now I'm going to leave that out.

[ZodiusInfuser]

Thanks for this PR @macifell! I haven't had chance to try this code out on hardware, but from looking over it I have a few comments / questions.

This fix allows configuration settings to be used to determine when and by how much to reduce the temperature. It also adjusts the relative humidity as this is dependent on the temperature.

Nice job! That should help a lot of the people who've been running on USB power, get more accurate readings.

Although there are two new config settings, only one is intended to be modified manually - the usb_power_temperature_offset, since I'm not sure how much it will vary between models and under different circumstances.

Config.py is intended for user configurable settings only. Although I get the idea of using it to store the usb state, the better way to do this (for this project anyway) would be to create a new file, which if present tells you the information you need. You can see several examples of this in the code, such as the one for reattempting upload: https://github.com/pimoroni/enviro/blob/8c33384b6219484dbd71960953807fece01fded0/enviro/__init__.py#L428-L430 https://github.com/pimoroni/enviro/blob/8c33384b6219484dbd71960953807fece01fded0/enviro/__init__.py#L495-L505 That being said...

There might be a better way to easily determine if we're on battery power vs. USB.

At startup you can check the value of vbus_present to determine if the board was powered up from USB. That could help you get around your "one reset iteration" issue. Perhaps this could let you remove the need for file writing at all, but then perhaps knowing both the usb state at the end of the last run, and at the start of the current one, would help in your logic?

I'm not sure what the policy is for adding options to the config file. These changes would either require that a user runs the setup process again or manually adds entries to the config file. A method for detecting and adding missing config settings could be added if it seems like that would be helpful, but for now I'm going to leave that out.

For now, any additions to config.py should be treated as optional features. This is so that users buying new Enviro's are able to update them from factory firmware and have them still function without the need to reprovision. Therefore any new config variables should have try clauses around them to check for their existence. Look at the mqtt file for an example of how to handle this for your usb_power_temperature_offset: https://github.com/pimoroni/enviro/blob/a9e2eb9d700982a0137e7dcb79394684fec3f65a/enviro/destinations/mqtt.py#L17-L20

One thing missing from that mqtt example is any log message to encourage the user to update their config.py. You're welcome to add this to your PR 🙂

Hope this feedback is useful for you.

[macifell]

Thank you for all the comments @ZodiusInfuser ! I'm glad to hear there's a better way to check for battery power and I think that will help simplify this code. The policy on configuration settings also makes perfect sense and I'll update this PR based on your feedback.

[macifell]

@ZodiusInfuser When you have a chance let me know what you think of these changes.

I put the config setting checks in a central place to see how that would look. Let me know if you think that's a good idea, or if you'd rather keep them separate.

Also, let me know if you'd like me to add the same logic for the other boards. I am reluctant to do this, since I don't have those models and can't test this code on them. It's possible each model might need it's own default offset value, I'm just not sure.

I've been running this code for a few days on my enviro indoor, switching back and forth between battery and usb power and the readings look very consistent to me - both temperature and humidity. They also agree with another sensor I have placed nearby.

[ZodiusInfuser]

Thanks for making those changes. I haven't had chance to test the code, but it's looking pretty good to me now. My only thought is if the call to add_missing_config_settings() should be moved up to around line 80, where config is first imported? It doesn't seem to depend on any parameters that get added to the code later on, unless I'm just not seeing it?

[ZodiusInfuser]

Line 94 perhaps. After the provisioning has taken place?

[macifell]

Line 94 perhaps. After the provisioning has taken place?

That's a great point. It does seem like this could run during the initial import, instead of in the startup method. I was a bit wary of running this code too early, but I think we should be safe running it right after the provisioning check.

[ZodiusInfuser]

This is looking good now @macifell! I'll try and make some time to test it next week, but give me a nudge if you don't hear anything.

[macifell]

Thank you @ZodiusInfuser! Let me know how it goes.

[ZodiusInfuser]

This has been tested on an enviro in our office and is giving readings that more closely match its neighbouring temperature sensors. I'll go ahead and merge this now. Thanks again @macifell!