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

Fixes #3074 and some other minor issues, improves battery level #3163

Closed
wants to merge 23 commits into from
Closed

Fixes #3074 and some other minor issues, improves battery level #3163

wants to merge 23 commits into from

Conversation

Gabrielerusso
Copy link
Contributor

@Gabrielerusso Gabrielerusso commented Feb 2, 2024

Fixes #3074 of heltec tracker, i found other issues such as the I2C pin not specified and the stock 21/22 were used, but pin 22 is not available, now using 45/46. Also the screen dimension and offsets now are correct.

A major difference was to split the two hardware version, to reduce overhead code as all the new boards are V1.1 and to me it made little sense to have such differentiations at runtime. The two hardware version have the same "external" name, just two different binaries.
new env names:

  1. heltec-wireless-tracker-V1_0
  2. heltec-wireless-tracker-V1_1

NOTE: i2c pin fix was never tried, original i2c pin are reported as 41/42 but that was false as those are used for the TFT SPI

Fixed screen timeout bug, also divided in two the compiled versions for heltec tracker hardwae v1.0 and 1.1, much cleaner and smaller code, nowdays every pcb version is V1.1
fixed screen offsets and i2c pins
@CLAassistant
Copy link

CLAassistant commented Feb 2, 2024

CLA assistant check
All committers have signed the CLA.

@Gabrielerusso
Copy link
Contributor Author

Added lookup table for a better battery level estimation, using a 11 value table and interpolating between them, for the moment shoould be good enough, if you wan't i could add more, but 11 values in my test (during BMS development) seemed good enough.
Still the automatic heltec tracker build fails as the new envs are:

  • heltec-wireless-tracker-V1_0
  • heltec-wireless-tracker-V1_1

@Gabrielerusso Gabrielerusso changed the title Fixes #3074 and some other minor issues Fixes #3074 and some other minor issues, improves battery level Feb 6, 2024
Updated battery open circuit voltage table, laso added some comments and updated heltec tracker framerate to 3
fixes location leaks when location is disabled and we don't want our position to be given.
@thebentern
Copy link
Contributor

@Gabrielerusso couple of things before we move forward. Can you sign the CLA and make the protobuf changes in a PR in the protobufs repo first?

src/Power.cpp Outdated Show resolved Hide resolved
removed delay from adc reading, added a software filter to smooth out voltage readings. In those applications battery would last hours to days, no sudden change should be expected so a less frequent voltage reading or a more aggressive filtering could be done.
Note: to speed up convergence i initiliazied the last value to the minimum voltage, there are other and better ways to init the filter.
@Gabrielerusso
Copy link
Contributor Author

@Gabrielerusso couple of things before we move forward. Can you sign the CLA and make the protobuf changes in a PR in the protobufs repo first?

CLA signed, I made a request for the updated protobuf and I hope it was correct. Pretty busy in this period and I'm reading/writing when I've a minute so I may have done something wrong with protobuf that I'm unfamiliar.

line 230/386 For heltec v3 and heltec tracker a different approach was used with the ADC_CTRL pin, now is more uniform using the same code for the 3 boards.

line 236 Check if the raw reading we are getting is Valid or not, count only the valid readings. This could lead to a division by 0 (improbable) so that's why at line 258 there is a check for that.
Fixed ESP32 ADC resolution bug introduced by #3184 as esp32 analog resolution is already set some line of code before to 12 bit default.
For our usage wouldn't be faster to use 10 bit? .
formatting corrections
@Gabrielerusso
Copy link
Contributor Author

I know those are starting to be a lot of commit and file changed for a single PR, maybe it would've been better to do so in multiple PR.

src/Power.cpp Outdated
delay(10);
#endif
for (int i = 0; i < BATTERY_SENSE_SAMPLES; i++) {
raw += adc1_get_raw(adc_channel);
int val_ = adc1_get_raw(adc_channel);
if(val_ >= 0){ //save only valid readings
Copy link
Contributor Author

@Gabrielerusso Gabrielerusso Feb 11, 2024

Choose a reason for hiding this comment

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

Check for valid readings, invalid should give -1 but could be usefull to use strictly greather than 0 instead of >= ?

pinMode(ADC_CTRL, OUTPUT);
digitalWrite(ADC_CTRL, HIGH);
digitalWrite(ADC_CTRL, ADC_CTRL_ENABLED);
delay(10);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

if there aren't filter caps with the resistor value i've seen the fet should saturate in less than 3ms

@code8buster
Copy link
Contributor

I know those are starting to be a lot of commit and file changed for a single PR, maybe it would've been better to do so in multiple PR.

Generally yes lol it does make it easier to merge when it's not all wrapped up. I'll spend some time this evening reviewing the changes.

- added possibility to completely disable DEBUG LOG
- fixed some warnings due to missing {} , the code has a lot of missing brackets in if statements, this is a bad habit.
- in file SerialConsole.cpp a conditional compiling condition was missing, making impossible to compile if no DEBUG_PORT was specified
@Gabrielerusso
Copy link
Contributor Author

I know those are starting to be a lot of commit and file changed for a single PR, maybe it would've been better to do so in multiple PR.

Generally yes lol it does make it easier to merge when it's not all wrapped up. I'll spend some time this evening reviewing the changes.

I'm step-by step trying to divide the changes in different branches to do multiple PR

@Gabrielerusso
Copy link
Contributor Author

i'm keeping this as draft for a personal reference, i did the first PR, once that get reviewed i'll make another one, also i'm removing all the "Merge remote-tracking branch 'upstream/master'" by doing a rebase from time to time. This should make the commits much easier to understand.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Heltec Tracker Screen Timeout[Bug]:
4 participants