mmyster/feature/lk core touch sensor #947
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## develop #947 +/- ##
========================================
Coverage 96.04% 96.04%
========================================
Files 133 133
Lines 3183 3183
========================================
Hits 3057 3057
Misses 126 126
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
File comparision analysis report🔖 Info
Click to show memory sections📝 SummaryClick to show summary
🗺️ Map files diff outputClick to show diff list
|
File comparision analysis report🔖 Info
Click to show memory sections📝 SummaryClick to show summary
🗺️ Map files diff outputClick to show diff list
|
MMyster
changed the base branch from
develop
to
mmyster/feature/add-core-touch-sensor
MMyster
force-pushed
the
mmyster/feature/add-core-touch-sensor
branch
2 times, most recently
from
ef0d612
to
a133b89
Compare
MMyster
force-pushed
the
mmyster/feature/lk-core-touch-sensor
branch
7 times, most recently
from
66ad008
to
d0b265d
Compare
MMyster
force-pushed
the
mmyster/feature/add-core-touch-sensor
branch
from
5df390b
to
5685ef2
Compare
YannLocatelli
requested changes
Jul 27, 2022
There was a problem hiding this comment.
Il y a quelque chose de majeur ici: ça ne marche pas à tous les coups
- le reset qui est parfois nécessaire, parfois non
- la calibration qui donne une valeur étonnante de aucune sensibilité comme toucher détecté
- un capteur qui se bloque à "Toucher" alors qu'on a plus la main dessus
C'est l'occasion idéale pour creuser et expliquer le pourquoi en ayant les cartes qu'il faut en main et sans en avoir trop non plus.
Note. Il s'agit là de soucis lié à la compréhension du matériel et non du code. La suite avec TouchSensorKit est donc tout à fait faisable.
MMyster
force-pushed
the
mmyster/feature/add-core-touch-sensor
branch
from
5685ef2
to
02c6812
Compare
|
MMyster
force-pushed
the
mmyster/feature/lk-core-touch-sensor
branch
from
d0b265d
to
3421be3
Compare
MMyster
force-pushed
the
mmyster/feature/add-core-touch-sensor
branch
from
02c6812
to
137ca2b
Compare
MMyster
force-pushed
the
mmyster/feature/lk-core-touch-sensor
branch
from
3421be3
to
11f1d7b
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
MMyster
force-pushed
the
mmyster/feature/add-core-touch-sensor
branch
from
137ca2b
to
12c4aed
Compare
MMyster
force-pushed
the
mmyster/feature/add-core-touch-sensor
branch
from
dae583a
to
24f6655
Compare
ladislas
force-pushed
the
mmyster/feature/add-core-touch-sensor
branch
from
24f6655
to
14245cc
Compare
Base automatically changed from
mmyster/feature/add-core-touch-sensor
to
develop
September 5, 2022 14:29
|
Please fix conflicts on this PR |
MMyster
force-pushed
the
mmyster/feature/lk-core-touch-sensor
branch
4 times, most recently
from
600917b
to
a76bd47
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
Sum-up existing bugs of TouchPrevious review
Peer-programming meeting
2 QDAC used for left and right sensors were inversed. Fixed in d5032d5. Moreover, calibration process is set at start of spike, and might consider the bug of sensor stuck to "Touched" at init (see #1112).
Might come from to too high sensiblity.
See |
MMyster
force-pushed
the
mmyster/feature/lk-core-touch-sensor
branch
4 times, most recently
from
829e762
to
b91f49a
Compare
YannLocatelli
approved these changes
Nov 14, 2022
ladislas
requested changes
Nov 15, 2022
spikes/lk_core_touch_sensor/main.cpp
Outdated
Comment on lines
136
to
155
| auto currentState = bool {false}; | ||
| while (true) { | ||
| currentState = sensor.read(); | ||
| if (currentState) { | ||
| log_info("Touched"); | ||
| } else { | ||
| log_info("."); | ||
| } | ||
| rtos::ThisThread::sleep_for(100ms); | ||
| } | ||
| } |
There was a problem hiding this comment.
Suggested change
| auto currentState = bool {false}; | |
| while (true) { | |
| currentState = sensor.read(); | |
| if (currentState) { | |
| log_info("Touched"); | |
| } else { | |
| log_info("."); | |
| } | |
| rtos::ThisThread::sleep_for(100ms); | |
| } | |
| } | |
| while (true) { | |
| auto is_touched = sensor.read(); | |
| if (is_touched) { | |
| log_info("Sensor touched"); | |
| } else { | |
| log_info("."); | |
| } | |
| rtos::ThisThread::sleep_for(100ms); | |
| } | |
| } |
spikes/lk_core_touch_sensor/main.cpp
Outdated
| auto resetByTouchAndRelease() | ||
| { | ||
| log_info("Put your hand on the sensor then release !\n\n"); | ||
| rtos::ThisThread::sleep_for(100ms); |
There was a problem hiding this comment.
100ms is too short for the sensor to "not work" and allow for reset by touch
Suggested change
| rtos::ThisThread::sleep_for(100ms); | |
| rtos::ThisThread::sleep_for(1000ms); |
| log_info("Put your hand on the sensor then release !\n\n"); | ||
| rtos::ThisThread::sleep_for(100ms); | ||
| while (sensor.read()) { | ||
| rtos::ThisThread::sleep_for(100ms); |
There was a problem hiding this comment.
Suggested change
| rtos::ThisThread::sleep_for(100ms); | |
| log_info("Waiting for hand..."); | |
| rtos::ThisThread::sleep_for(100ms); |
spikes/lk_core_touch_sensor/main.cpp
Outdated
| sensor.init(); | ||
| sensor.setSensitivity(0x0FF0); | ||
|
|
||
| resetByTouchAndRelease(); // To continue with the bug |
There was a problem hiding this comment.
Suggested change
| resetByTouchAndRelease(); // To continue with the bug | |
| // ? Current workaround for bug #1112 | |
| // TODO(@leka/dev-embedded) - Try to find solution w/ software or ui/ux | |
| resetByTouchAndRelease(); |
| rtos::ThisThread::sleep_for(2s); | ||
|
|
||
| sensor.init(); | ||
| sensor.setSensitivity(0x0FF0); |
There was a problem hiding this comment.
this number doesn't mean much for someone reviewing the code. Is it a magic number? Is it "au doigt mouillé"? Is it high sensitivity or low sensitivity? why 0x0FF0 and not 0x0FF1?
solutions:
- create an
enum class Sensitivitywith different levels:low/medium/highso that it's easier to set/understand - use a named constant for the value -->
auto kDefault{High/Low}Sensitivity = 0x0FF0; - have
setSensitivity(value)take a float -->setSensitivity(float value)with values between 0.0 (super low sensitivity) to 1.0 (super high sensitivity) and do thefloat --> uint16_tmapping in the implementation
I favor 2. and 3. and they can be combined
This can be implemented in a followup PR.
MMyster
force-pushed
the
mmyster/feature/lk-core-touch-sensor
branch
2 times, most recently
from
ae43f7c
to
8fe0d09
Compare
ladislas
force-pushed
the
mmyster/feature/lk-core-touch-sensor
branch
from
8fe0d09
to
22e8006
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Need :