pimoroni / enviro

MIT License
105 stars 84 forks source link

Re-Enable the battery monitoring on Enviro! 🥳 #146

Open PascalKern opened 1 year ago

PascalKern commented 1 year ago

As mentioned in the release note of v0.0.9 the battery voltage monitoring was disabled due to issues with the stability

One of the big causes of instability of past versions came from the introduction of battery monitoring. The pin to do this is shared with the Pico W's WiFi chip, and it seems there is no 100% safe way to read this without causing connection or other communication issues. Unfortunately the only way to get around this, for now, was to disable the battery monitoring on Enviro 😢

This pull request will solve this after I tested it as well as possible to be more or less 100% sure that the issue is gone.

Credit Note: The solution does (unfortunately) not origin in my own knowledge but was more or less inspired and then a partial C&P action from the following source(s) and super great work of Daniel Peron

The current solution was tested on my Enviro devices. Below is the list of when the patch was applied ie. since when the patch runs on each device(s). All the devices were using the pimoroni-picow_enviro-v1.19.10-micropython-v0.0.9.uf2 OS as the base.

As this feature created a lot of issues in the past I added a feature toggle to the config so that the voltage monitoring can simply be deactivated in case one experience stability issues again with the feature enabled.

Important: In the config_template.py is the feature DISABLED by default!

ZodiusInfuser commented 1 year ago

Hi @PascalKern. Thanks for raising this. You're proposed solution is interesting, as your sources are the exact two things I tried but still had issues with. Perhaps I didn't implement them correctly??

I will need to set up my own tests for this code to check if it works, though I do very much appreciate the inclusion of the config parameter! That makes the risk of merging this much lower than would otherwise be the case. I just need to check how you are adding that parameter, as additions to config_template.py should be treated as optional, so users can safely upgrade without re provisioning.

Since I applied the patch to these devices on the given dates I never had any instability issues! 🤩

When you mention no instability issues, I assume you are still experiencing the other know lock-ups, just not any that are obviously attributed to vsys measurement?

PascalKern commented 1 year ago

Hi @ZodiusInfuser

First of all sorry for the late reply. Sometimes life (or just too much work) comes in the way when not expected.

Regarding the additions to the config_template.py, I took care of backward compatibility. I had the same issue with my other devices while developing and testing the solution on just one of them. :) But more checks are always better than fewer. But thanks for appreciating the feature flag. As you said, I want that failing feature to be able to disable.

For the no instability issues I mentioned I can only tell about the issues I had before the implementation. I only had some in earlier versions of the FW but I'm also only using MQTT as a destination. Additionally is my WiFi quite poorly configured/setup and some issues in the past were related to bad/weak connectivity. But since the dates provided in my comment, I never had any of the previous issues only once a connection loss but this was due to a battery pack on the lower end of the needed voltage ie. almost below what is needed AND this was the weather sensor ie. outdoor and with the cold(er) weather (here it was between -7 to 3°C) back in these days this was also not helpful. So I took this issue as the combination of the low battery voltage, cold weather and the sensor being outdoors which does also not help for my weak wifi. ;)

Anyway. I appreciate any further, most likely deeper and dedicated, testing for that feature.

I'm going to rebase the branch anyway to not lag to much behind the main branch in case of a future merge.

Best, Pascal

arcticash commented 9 months ago

Hope this gets merged into the main codebase soon!

I've just manually implemented the changes and it all seems to be working fine, fingers crossed this project hasn't been abandoned by Pimoroni!

Gadgetoid commented 5 months ago

A rebase would be very much appreciated here, if you have the time and inclination!