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

Adding support for MAX17043 Lipo battery-gauge #18739

Closed
wants to merge 17 commits into from
Closed

Adding support for MAX17043 Lipo battery-gauge #18739

wants to merge 17 commits into from

Conversation

Vincent1964
Copy link
Contributor

@Vincent1964 Vincent1964 commented May 29, 2023

Description:

This PR contains a sensor driver for MAX17043 a battery gauge for 3.7 Volt batteries. Allows to monitor a battery powered ESP.

Related issue (if applicable): fixes - NONE

Checklist:

  • The pull request is done against the latest development branch
  • Only relevant files were touched
  • Only one feature/fix was added per PR and the code change compiles without warnings
  • The code change is tested and works with Tasmota core ESP8266 V.2.7.4.9
  • The code change is tested and works with Tasmota core ESP32 V.2.0.9
  • I accept the CLA.

NOTE: The code change must pass CI tests. Your PR cannot be merged unless tests pass

@Vincent1964
Copy link
Contributor Author

First PR, hope I did everything the way it should be.

Copy link
Contributor Author

@Vincent1964 Vincent1964 left a comment

Choose a reason for hiding this comment

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

already fixed by: 18737

.vscode/settings.json Outdated Show resolved Hide resolved
#ifdef USE_WEBSERVER
void Max17043Show(void) {

WSContentSend_P(PSTR("Battery: %.1f V, "), max17043->voltage/1000);
Copy link
Owner

Choose a reason for hiding this comment

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

  • Use standard Tasmota GUI layout (Search code for examples)
  • Use standard Tasmota %1_f instead of not supported (because adds loads of unneeded code size) %.1f

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

void Max17043Show(void) {

WSContentSend_P(PSTR("Battery: %.1f V, "), max17043->voltage/1000);
WSContentSend_P(PSTR("%.1f %%"), max17043->percentage);
Copy link
Owner

Choose a reason for hiding this comment

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

Ditto. Search for HTTP_BATTERY for an example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

void Max17043Json(void) {

ResponseAppend_P(JSON_SNS_BGAUGE, mqttId, String(max17043->voltage/1000, 2), String(max17043->percentage, 1));
}
Copy link
Owner

Choose a reason for hiding this comment

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

Don't use String() as in takes way too much code (size). See other drivers how to use %2_f.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed


void Max17043Init(void) {

if (gauge.begin() != 0) {
Copy link
Owner

Choose a reason for hiding this comment

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

Add I2cSetDevice() to test avialable I2C Address.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

AddLog(LOG_LEVEL_ERROR, PSTR("I2C: MAX17043 not found"));
} else {
max17043 = (MAX17043 *)calloc(1, sizeof(struct MAX17043));
AddLog(LOG_LEVEL_INFO, PSTR("I2C: MAX17043 initialized"));
Copy link
Owner

Choose a reason for hiding this comment

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

Remove AddLog and add I2cSetActiveFound() claiming it's I2C address.

Your driver needs to integrate in Tasmota's I2C eco-system.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link
Owner

Choose a reason for hiding this comment

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

No need to change anything in this file. Remove from your PR

Copy link
Owner

Choose a reason for hiding this comment

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

Already merged. Remove from your PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, will be removed


#ifdef USE_WEBSERVER
void Max17043Show(void) {
WSContentSend_PD(PSTR("{s}%s Voltage {m}%1_f V {e}"), SENSOR_NAME, &max17043->voltage);
Copy link
Owner

Choose a reason for hiding this comment

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

Pls chg "Voltage" with defined D_VOLTAGE making it multi language.

Add "Capacity" as english word to all language files; translations will eventually be done by others.

Copy link
Owner

Choose a reason for hiding this comment

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

By removing this file the PR fails initial chks. Pls find another solution to fix PR and making it ready to merge.

@Vincent1964 Vincent1964 marked this pull request as draft June 3, 2023 13:37
modified:   tasmota/language/bg_BG.h
modified:   tasmota/language/ca_AD.h
modified:   tasmota/language/cs_CZ.h
modified:   tasmota/language/de_DE.h
modified:   tasmota/language/el_GR.h
modified:   tasmota/language/en_GB.h
modified:   tasmota/language/es_ES.h
modified:   tasmota/language/fr_FR.h
modified:   tasmota/language/fy_NL.h
modified:   tasmota/language/he_HE.h
modified:   tasmota/language/hu_HU.h
modified:   tasmota/language/it_IT.h
modified:   tasmota/language/ko_KO.h
modified:   tasmota/language/nl_NL.h
modified:   tasmota/language/pl_PL.h
modified:   tasmota/language/pt_BR.h
modified:   tasmota/language/pt_PT.h
modified:   tasmota/language/ro_RO.h
modified:   tasmota/language/ru_RU.h
modified:   tasmota/language/sk_SK.h
modified:   tasmota/language/sv_SE.h
modified:   tasmota/language/tr_TR.h
modified:   tasmota/language/uk_UA.h
modified:   tasmota/language/vi_VN.h
modified:   tasmota/language/zh_CN.h
modified:   tasmota/language/zh_TW.h
modified:   tasmota/tasmota_xsns_sensor/xsns_109_max17043.ino

#ifdef USE_WEBSERVER
void Max17043Show(void) {
WSContentSend_PD(PSTR("{s}%s D_VOLTAGE {m}%1_f V {e}"), SENSOR_NAME, &max17043->voltage);
Copy link
Owner

Choose a reason for hiding this comment

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

Getting close! Look how they did it here: "{s}%s " D_VOLTAGE "{m}%1_f " D_UNIT_VOLT "{e}". Notice the quotes.

#ifdef USE_WEBSERVER
void Max17043Show(void) {
WSContentSend_PD(PSTR("{s}%s D_VOLTAGE {m}%1_f V {e}"), SENSOR_NAME, &max17043->voltage);
WSContentSend_PD(PSTR("{s}%s D_BATTERY_CAPACITY {m}%1_f %% {e}"), SENSOR_NAME, &max17043->percentage);
Copy link
Owner

Choose a reason for hiding this comment

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

Ditto quotes.

Once changed pls test your code as this error should have been noticed by you too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it worked, I normaly test before PR. Anyway, thanks for your patience with me!!

@Jason2866
Copy link
Collaborator

.vscode/settings.json is still deleted with your PR, and you are doing changes in files not related with your PR. Please update your branch with latest development and remove the not needed changes in files.

@Vincent1964
Copy link
Contributor Author

To do that I have to revoke my PR, update and make a new PR, right?

Thanks for your patience!

@Vincent1964 Vincent1964 requested a review from arendst June 3, 2023 15:51
@Jason2866
Copy link
Collaborator

Probably easiest is to close this PR and open a new one.

@Vincent1964
Copy link
Contributor Author

For me yes but not for the Theo and other reviewers they then have to review all my code again.. I will try to remove the file from the PR

@arendst
Copy link
Owner

arendst commented Jun 3, 2023

I agree with jason.

Take your time anyway as I won’t be able to merge the next fifteen days.

@Vincent1964
Copy link
Contributor Author

Okay Theo, relax

@Vincent1964 Vincent1964 deleted the branch arendst:development June 4, 2023 07:58
@Vincent1964 Vincent1964 closed this Jun 4, 2023
@Vincent1964 Vincent1964 deleted the development branch June 4, 2023 07:58
@Vincent1964 Vincent1964 restored the development branch June 4, 2023 08:02
@sfromis sfromis mentioned this pull request Jun 4, 2023
6 tasks
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.

3 participants