Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

PascalKern
Copy link

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.

  • 02.01.2023 1x Weather
  • 15.01.2023 1x Grow
  • 20.01.2023 1x Indoor (one of four)
  • 23.01.2023 3x Indoor (rest of them)
  • 23.01.2023 1x Urban
    Since I applied the patch to these devices on the given dates I never had any instability issues! 🤩

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
Copy link
Member

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
Copy link
Author

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

@@ -0,0 +1,30 @@
import machine

ADC_VOLT_CONVERSATION = 3.3 / 65535
Copy link

@LionsPhil LionsPhil Oct 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ADC_VOLT_CONVERSATION = 3.3 / 65535
ADC_VOLT_CONVERSION = 3.3 / 65535

(And below, appears to be a sneaky typo. :) I hope this gets merged for v0.0.11!)

@arcticash
Copy link

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
Copy link
Member

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

@Gadgetoid Gadgetoid added enhancement New feature or request bug fix labels Apr 24, 2024
@Gadgetoid Gadgetoid added this to the 1.0.0 milestone Apr 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug fix enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants