Add vl53l1x support #15566
Closed
Add vl53l1x support #15566
Conversation
This file contains hidden or 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
|
Found the following issues, please fix and resubmit: License issuesIn most cases you do not need to do anything here, especially if the files
|
overheat
force-pushed
the
add_vl53l1x_support
branch
3 times, most recently
from
e71cac7 to
86d1487
Compare
dbkinder
suggested changes
Apr 21, 2019
samples/sensor/vl53l1x/README.rst
Outdated
There was a problem hiding this comment.
measures the distance between the VL53L1X sensor and a target
samples/sensor/vl53l1x/README.rst
Outdated
There was a problem hiding this comment.
Thank you! Already changed.
overheat
force-pushed
the
add_vl53l1x_support
branch
from
86d1487 to
418b6f4
Compare
MaureenHelm
requested changes
Apr 22, 2019
There was a problem hiding this comment.
Please review https://docs.zephyrproject.org/latest/contribute/index.html#contributing-non-apache-2-0-licensed-components and fill out the README template.
cc: @avisconti
There was a problem hiding this comment.
@avisconti Thanks!
I have a question that those code under ext/hal/st/ comes from ST. Can I modify the copyright part as vl53l0x does?
There was a problem hiding this comment.
@overheat
For the ext part yes. You may refer to the existing VL53L0X for reference.
For the driver/sensor/vl53l1x you may change/add ur name and 2019 in the copyright section.
I think you may need to change CODEOWNER to cover that code. You should be the owner for the driver, while I think that someone from ST should for what concern the ext part (maybe myself or @erwango)
There was a problem hiding this comment.
@avisconti Thank you! I will update it. I will put you the CODEOWNER for the ext part, is that alright?
There was a problem hiding this comment.
So, at the end I think that you won't need to change CODEOWNERS at all.
avisconti
requested changes
Apr 23, 2019
drivers/sensor/vl53l1x/Kconfig
Outdated
There was a problem hiding this comment.
I guess here you may want to add a new copyright (2019).
Anyway, I was thinking whether we ca re-use same driver structure for both VL53L0X and VL53L1X. They seem
similar.
There was a problem hiding this comment.
As I was saying before do you think it is possible to re-use same routine for both VL53L1 and VL53L0?
Maybe using ifdef for the difference.
There was a problem hiding this comment.
Same here.
What is the difference with VL53L0?
index is on 16-bit instead of 8-bit?
There was a problem hiding this comment.
Yes, the VL53L1 index is on 16-bit instead of 8-bit
I have thought about re-use same driver but looks the internal implication is different between vl53l1x and vl53l0x, as you already have seen the "index"(register address) length is not the same. Also, those API related to interrupt and threshold and so on. So there should be lots of ifdef, if we want to merge those drivers.
There was a problem hiding this comment.
I have thought about re-use same driver but looks the internal implication is different between vl53l1x and vl53l0x, as you already have seen the "index"(register address) length is not the same. Also, those API related to interrupt and threshold and so on. So there should be lots of ifdef, if we want to merge those drivers together.
Yes, yes. In fact my initial idea was to re-use same driver, but the more I was going deep thru the review the less I found it applicable. So at the end I left it as a comment from your side.
|
@avisconti @MaureenHelm @dbkinder I have modified. Please review. Thanks! |
overheat
force-pushed
the
add_vl53l1x_support
branch
from
da8c77c to
27f64fd
Compare
erwango
requested review from
MaureenHelm,
avisconti and
dbkinder
and removed request for
MaureenHelm and
avisconti
Well, if I'm getting the change I guess this is not what we were expecting. You can take as an example the PR #14111 and especially the 52bc149 commit. You will see commit 52bc149f0787e51a3ee44ad7833fec50cb0e34a1 Moreover, I also added a new README file: ext/hal/st/stmemsc/README (see again #14111) So, to summarize, you have to keep the ext code original, then add a README file as I did, then Instead, what I asked you to change is the copyright of the drivers/sensor/vl53l1x driver. Does it make sense to you? |
dbkinder
approved these changes
Apr 29, 2019
overheat
force-pushed
the
add_vl53l1x_support
branch
from
27f64fd to
0df07f8
Compare
|
@avisconti Thank you for your patience. I have restored the ext code back to ST code original and add a README file, also modify the info in the commit msg. meanwhile, change the copyright of the drivers/sensor/vl53l1x driver. |
avisconti
requested changes
May 3, 2019
There was a problem hiding this comment.
We are almost done. See my latest comment here about using 'imply' in Kconfig for NEWLIB_LIBC.
Moreover, can you squash together the related commits?
At the end I think that you should have only 3 commits:
- ext: Add official ST library for vl53l1x
- drivers: sensor: Add vl53l1x time of flight sensor driver
- samples: sensor: Add vl53l1x time of flight sensor sample
Does it make sense?
ext/hal/st/lib/sensor/vl53l1x/README
Outdated
There was a problem hiding this comment.
I'm currently adding a similar external hal from ST Microelectronics for other sensors. I also need to
define NEWLIB_LIBC, but instead of letting the user to define it in prj.conf I used imply in Kconfig of the external
code. Have you considered to do the same?
See #14111 and in particular a40e62975 commit.
There was a problem hiding this comment.
yes, sound better.
wait a moment, I will push again.
There was a problem hiding this comment.
OK, just remember to squash together similar commits as I was saying before.
I suggest to save the branch before squashing just in case
This package contains platform independent drivers written in C language for STMicroelectronics ToF sensors VL53L1X. The aim of this package is to provide a common, clean and stable interface to get ranging data. Library is located in ext/hal/st/lib/sensor/vl53l1x Origin: ST Microelectronics License: BSD-3-Clause URL: http://www.st.com/en/embedded-software/stsw-img007.html Commit: v2.3.3 Purpose: provide a common and stable i/f to get ranging data Maintained-by: ST Microelectronics Signed-off-by: Aaron Tsui <[email protected]>
Contains vl53l1x driver itself and the adaptation layer, which is implemented in the driver as it is Zephyr specific. Support its own thread (minimum latency) or share a system-wide thread (less memory) to fetch data. Also, need to clear interrupt flag after every measurement on vl53l1x. Add dts yaml file as dts/bindings/sensor/st,vl53l1x.yaml Signed-off-by: Aaron Tsui <[email protected]>
overheat
force-pushed
the
add_vl53l1x_support
branch
from
0df07f8 to
c3bd67e
Compare
overheat
force-pushed
the
add_vl53l1x_support
branch
from
c3bd67e to
66d27b8
Compare
|
Fix minor bugs. |
vl53l1x time of flight (TOF) sensor sample, fetching proximity and distance data in every second. Add overlay file for nucleo_f401re to make this sample works out-of-box. Signed-off-by: Aaron Tsui <[email protected]>
overheat
force-pushed
the
add_vl53l1x_support
branch
from
66d27b8 to
f91f0ca
Compare
@avisconti yes, should I wait until that PR merged then re-trigger this CI check again? Sent with GitHawk |
Maybe you can get my commit (c18747f) from my remote repo on github, rebase on top of it and re-push everything: git add remote avisconti https://github.com/avisconti/zephyr.git I think it is important to know if it is fixing or not. |
The LL_I2S_ReadReg() function returns uint32_t, while %d requires 'int' as a type. Signed-off-by: Armando Visconti <[email protected]>
Done, let's see what happen. |
|
@avisconti it works. I will push again when #15857 be merged. Sent with GitHawk |
|
superseeded by #18772 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
VL53L1X time of flight (TOF) sensor supporting
https://www.st.com/en/imaging-and-photonics-solutions/vl53l1x.html
The vl53l1 library
To ease the usage of its vl53l1x time of flight sensor, STMicroelectronics provides this library, which is made of 2 parts :
The vl53l1x driver
Contains vl53l1x driver itself and the adaptation layer, which is implemented in the driver as it is Zephyr specific.
The vl53l1x sample
vl53l1x sensor sample, fetching proximity and distance data in every second.
Misc
documentation and overlay file