Get battery levels #281

Kabroc · 2021-06-22T13:10:10Z

Resistors value from documentation doesn't fit the actual value of resistors in the robot, until I know those values the coefficient is 0.125 which correspond to the actual coefficient of the voltage divider.

ladislas

First review. 👍

drivers/LKCoreBattery/include/LKCoreBattery.h

drivers/LKCoreBattery/source/LKCoreBattery.cpp

codecov · 2021-06-24T13:54:45Z

Codecov Report

Merging #281 (7b4b22c) into develop (8c52008) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 7b4b22c differs from pull request most recent head 62fd075. Consider uploading reports for the commit 62fd075 to get more accurate results

@@            Coverage Diff            @@
##           develop      #281   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           44        44           
  Lines          874       878    +4     
=========================================
+ Hits           874       878    +4

Impacted Files	Coverage Δ
drivers/CoreBattery/include/CoreBattery.h	`100.00% <100.00%> (ø)`
drivers/CoreBattery/source/CoreBattery.cpp	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8c52008...62fd075. Read the comment docs.

ladislas

2ème review, on est presque bon :)

drivers/CoreBattery/include/CoreBattery.h

drivers/CoreBattery/source/CoreBattery.cpp

YannLocatelli

Il faut revoir les noms des namespaces et discuter de la pertinence des Levels dans Core plutôt que dans Kit

drivers/CoreBattery/include/CoreBattery.h

drivers/CoreBattery/tests/CoreBattery_test.cpp

drivers/CoreBattery/source/CoreBattery.cpp

drivers/CoreBattery/include/CoreBattery.h

drivers/CoreBattery/tests/CoreBattery_test.cpp

ladislas

Modulo les derniers commentaires, on va être bon.

le seul truc qu'il faut faire c'est avoir deux commits:

premier pour le renaming de LKCoreBattery à CoreBattery
deuxième pour le code

prendre exemple sur les autres commits de renaming pour voir comment écrire le message.

YannLocatelli

Beaucoup de choses sont parties depuis la dernière review, j'avoue être un peu déconcerté.

La première chose qui me perturbe c'est le retrait des informations propre au matériel: R1 et R2 ou du moins le coefficient du PdT. Sans ça je ne vois pas comment tu peux relier ton float [0, 1] à la tension de ta batteries [0, 13.20V].

La seconde chose c'est dans l'utilisation, si j'appelle getRemainingVoltage j'ai une valeur entre 0 et 1. Si j'ai 1.0, est ce que ça signifie qu'il reste à la batterie 1.0V?

--

Je sais qu'il y avait litige avec les levels, parce qu'ils étaient subjectifs, mais tout ce qui est lié aux conversions sont objectifs, je ne peux pas obliger R1 à être à 42Ω par exemple, c'est physiquement impossible.

Du coup quel est le plan pour la suite?

ladislas · 2021-07-02T10:41:10Z

Je sais qu'il y avait litige avec les levels, parce qu'ils étaient subjectifs, mais tout ce qui est lié aux conversions sont objectifs, je ne peux pas obliger R1 à être à 42Ω par exemple, c'est physiquement impossible.

Du coup quel est le plan pour la suite?

je rejoins @YannLocatelli sur ces points. tout ce qui est lié au matériel doit être présent dans le Core donc les conversions des résistances doivent y être.

La seconde chose c'est dans l'utilisation, si j'appelle getRemainingVoltage j'ai une valeur entre 0 et 1. Si j'ai 1.0, est ce que ça signifie qu'il reste à la batterie 1.0V?

à mon sens get remaining voltage doit te retourner la valeur en volt, donc 7,3 si c'est 7.3.

Si on veut que ce soit entre 0 et 1, dans ce cas là on parlera de pourcentage plutôt.

mais je ne me lancerai pas dans les % ici. le calcul il peut être fait sur l'iPad plutôt, il faut juste retourner la bonne valeur du voltage.

YannLocatelli

Tout me va dans le fonctionnement ✅✨

Par contre c'est un peu difficile à relire, je vais encore chipoter sur certains termes et sur la présentation du fonctionnement de la batteries dans les unit tests

drivers/CoreBattery/tests/CoreBattery_test.cpp

YannLocatelli · 2021-07-05T17:41:07Z

drivers/CoreBattery/tests/CoreBattery_test.cpp

+TEST(CoreBatteryTest, getVoltagePercentageHigherThan1)
+{
+	auto expected_digital_voltage	 = 0.9;
+	auto expected_percentage_voltage = 1.;
+
+	spy_AnalogIn_setValue(expected_digital_voltage);
+
+	ASSERT_EQ(expected_percentage_voltage, corebattery.getPercentage());
+}


C'est contre-intuitif, le nom du test nous dit qu'on va tester si c'est plus grand que 1, mais on voit nulle part une valeur (strictement) plus grand que 1.

Venant de l'extérieur c'est difficile à comprendre quel est la variable (s'il y en a une) qui sera plus grand que 1.

Le test c'est "s'assurer que pour une valeur qui devrait donner un pourcentage supérieur à 1 on ne dépasse pas 1", d'ou le fait qu'il n'y ait pas de valeur supérieure à 1 car on n'en veut pas.

YannLocatelli · 2021-07-05T17:58:11Z

drivers/CoreBattery/source/CoreBattery.cpp

+
+auto CoreBattery::getVoltage() -> float
+{
+	auto digitalVoltage = getADCVoltage();


Les deux noms ne (me) conviennent pas.

digitalVoltage parce qu'il est censé correspondre à la tension batterie or c'est la tension batterie avec division du pont diviseur de tension. Donc ça serait plus digitalDividedVoltage.
Sinon il est possible de le voir dans l'autre sens digitalInputVoltage.

getADCVoltage, ADC correspond à Analog-Digital Converter. C'est un outil et donc pas quelque chose qui se mesure.
Dans l'idée il devrait avoir un nom équivalent à la variable qui va get la valeur. Donc quelque chose comme auto digitalVoltage = getDigitalVoltage() ou auto digitalInputVoltage = getDigitalInputVoltage(), ça ferait plus de sens.

Soit dit en passant, les variables sont en snake_case, donc plutôt

auto digital_voltage = getDigitalVoltage(); // or auto digital_input_voltage = getDigitalInputVoltage();

YannLocatelli · 2021-07-05T18:01:35Z

drivers/CoreBattery/source/CoreBattery.cpp

+auto CoreBattery::readVoltage() -> float
+{
+	return _pin.read();
+}


Je vais encore être enquiquinant, je vais remettre le commentaire de la dernière fois:

La seconde chose c'est dans l'utilisation, si j'appelle getRemainingVoltage j'ai une valeur entre 0 et 1. Si j'ai 1.0, est ce que ça signifie qu'il reste à la batterie 1.0V?

Ici on se retrouve avec la même situation: si j'appelle readVoltage(), j'ai une valeur entre 0 et 1: ça signifie qu'il ne peut y avoir plus que 1V en entrée du MCU max?

Peut-être était-ce ici le read_voltage?

Suggested change

auto CoreBattery::readVoltage() -> float

{

return _pin.read();

}

auto CoreBattery::readVoltage() -> float

{

return _pin.read_voltage();

}

oui je suis d'accord, et de faire ça fix les problèmes de noms évoqués plus haut.

drivers/CoreBattery/include/CoreBattery.h

- Remove LK prefix from LKCoreBattery

- move namespance constexpr to class - Add voltage reference - Add max/min capacity values

sonarcloud · 2021-07-06T15:36:01Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
3 Code Smells

100.0% Coverage
0.0% Duplication

Kabroc added the 01 - type: task Something to do label Jun 22, 2021

Kabroc requested review from ladislas and YannLocatelli June 22, 2021 13:10

Kabroc self-assigned this Jun 22, 2021

Kabroc force-pushed the benjamin/feature/battery_levels branch from 0b3b839 to 3179404 Compare June 23, 2021 08:24

ladislas requested changes Jun 24, 2021

View reviewed changes

Kabroc force-pushed the benjamin/feature/battery_levels branch 3 times, most recently from fbfa4c0 to 65e72c1 Compare June 24, 2021 14:14

ladislas requested changes Jun 24, 2021

View reviewed changes

YannLocatelli requested changes Jun 25, 2021

View reviewed changes

Kabroc force-pushed the benjamin/feature/battery_levels branch from 41f45dc to ff514ec Compare June 25, 2021 13:51

ladislas reviewed Jun 26, 2021

View reviewed changes

drivers/CoreBattery/source/CoreBattery.cpp Outdated Show resolved Hide resolved

ladislas reviewed Jun 26, 2021

View reviewed changes

drivers/CoreBattery/include/CoreBattery.h Show resolved Hide resolved

ladislas reviewed Jun 26, 2021

View reviewed changes

drivers/CoreBattery/tests/CoreBattery_test.cpp Outdated Show resolved Hide resolved

ladislas reviewed Jun 26, 2021

View reviewed changes

drivers/CoreBattery/tests/CoreBattery_test.cpp Outdated Show resolved Hide resolved

ladislas requested changes Jun 26, 2021

View reviewed changes

Kabroc force-pushed the benjamin/feature/battery_levels branch from 79c9971 to 76f5798 Compare June 28, 2021 08:36

YannLocatelli reviewed Jul 1, 2021

View reviewed changes

Kabroc force-pushed the benjamin/feature/battery_levels branch from 33db171 to 3bf893c Compare July 2, 2021 13:26

ladislas force-pushed the benjamin/feature/battery_levels branch 2 times, most recently from 247d39c to d351541 Compare July 5, 2021 12:11

Kabroc force-pushed the benjamin/feature/battery_levels branch 3 times, most recently from e7546d2 to 0d2d320 Compare July 5, 2021 13:06

ladislas force-pushed the benjamin/feature/battery_levels branch from 0d2d320 to f1320bc Compare July 5, 2021 13:59

YannLocatelli requested changes Jul 5, 2021

View reviewed changes

Kabroc force-pushed the benjamin/feature/battery_levels branch from 012bc5b to e1c590a Compare July 6, 2021 08:36

ladislas force-pushed the benjamin/feature/battery_levels branch from e1c590a to f557d78 Compare July 6, 2021 08:54

ladislas requested changes Jul 6, 2021

View reviewed changes

drivers/CoreBattery/include/CoreBattery.h Outdated Show resolved Hide resolved

Kabroc force-pushed the benjamin/feature/battery_levels branch from d8664d0 to 7b4b22c Compare July 6, 2021 15:02

Kabroc added 2 commits July 6, 2021 17:26

🚚 (drivers): Rename LKCoreBattery to CoreBattery

200cc7e

- Remove LK prefix from LKCoreBattery

✨ (drivers): Refactor getVoltage method

62fd075

- move namespance constexpr to class - Add voltage reference - Add max/min capacity values

ladislas force-pushed the benjamin/feature/battery_levels branch from 7b4b22c to 62fd075 Compare July 6, 2021 15:30

ladislas merged commit 18926e4 into develop Jul 6, 2021

ladislas deleted the benjamin/feature/battery_levels branch July 12, 2021 09:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get battery levels #281

Get battery levels #281

Kabroc commented Jun 22, 2021

ladislas left a comment

codecov bot commented Jun 24, 2021 •

edited

Loading

ladislas left a comment

YannLocatelli left a comment

ladislas left a comment

YannLocatelli left a comment •

edited

Loading

ladislas commented Jul 2, 2021

YannLocatelli left a comment

YannLocatelli Jul 5, 2021

Kabroc Jul 6, 2021

YannLocatelli Jul 5, 2021 •

edited

Loading

YannLocatelli Jul 5, 2021

YannLocatelli Jul 5, 2021

ladislas Jul 6, 2021

sonarcloud bot commented Jul 6, 2021

Get battery levels #281

Get battery levels #281

Conversation

Kabroc commented Jun 22, 2021

ladislas left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 24, 2021 • edited Loading

Codecov Report

ladislas left a comment

Choose a reason for hiding this comment

YannLocatelli left a comment

Choose a reason for hiding this comment

ladislas left a comment

Choose a reason for hiding this comment

YannLocatelli left a comment • edited Loading

Choose a reason for hiding this comment

ladislas commented Jul 2, 2021

YannLocatelli left a comment

Choose a reason for hiding this comment

YannLocatelli Jul 5, 2021

Choose a reason for hiding this comment

Kabroc Jul 6, 2021

Choose a reason for hiding this comment

YannLocatelli Jul 5, 2021 • edited Loading

Choose a reason for hiding this comment

YannLocatelli Jul 5, 2021

Choose a reason for hiding this comment

YannLocatelli Jul 5, 2021

Choose a reason for hiding this comment

ladislas Jul 6, 2021

Choose a reason for hiding this comment

sonarcloud bot commented Jul 6, 2021

codecov bot commented Jun 24, 2021 •

edited

Loading

YannLocatelli left a comment •

edited

Loading

YannLocatelli Jul 5, 2021 •

edited

Loading