Skip to content

drivers: sensors: add ST VL53L1X TOF sensor #18772

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

drivers: sensors: add ST VL53L1X TOF sensor #18772

wants to merge 1 commit into from

Conversation

@chris-schra
Copy link
Contributor

@chris-schra chris-schra commented Aug 29, 2019

Adds Zephyr platform driver for VL51L1X TOF sensor,
based on ST's driver API (see UM2356)

@chris-schra
Copy link
Contributor Author

chris-schra commented Aug 29, 2019 •
edited
Loading

This is a driver for ST VL53L1X. I mostly took the code from vl53l0x driver but rewrote the platforms api to support 16bit register of vl53l1x (instead of 8bit of vl53l0x).
My most important question is: how do I create a pull request against the /hal/st module? how do I reference it here?
And what is the current best practice for allowing multiple device instances?

@zephyrbot zephyrbot added area: Devicetree area: Sensors Sensors area: Tests Issues related to a particular existing or missing test labels Aug 29, 2019
@zephyrbot
Copy link

zephyrbot commented Aug 29, 2019 •
edited
Loading

Found the following issues, please fix and resubmit:

License issues

In most cases you do not need to do anything here, especially if the files
reported below are going into ext/ and if license was approved for inclusion
into ext/ already. Fix any missing license/copyright issues. The license
exception if a JFYI for the maintainers and can be overriden when merging the
pull request.

  • drivers/sensor/vl53l1x/vl53l1_platform.c is not apache-2.0 licensed: free-unknown
  • drivers/sensor/vl53l1x/vl53l1_platform.c has non-permissive license: free-unknown
  • drivers/sensor/vl53l1x/vl53l1_platform.c is not apache-2.0 licensed: proprietary-license
  • drivers/sensor/vl53l1x/vl53l1_platform.c has non-permissive license: proprietary-license
  • drivers/sensor/vl53l1x/vl53l1_platform.c is not apache-2.0 licensed: bsd-new
  • drivers/sensor/vl53l1x/vl53l1_platform.c is not apache-2.0 licensed: proprietary-license
  • drivers/sensor/vl53l1x/vl53l1_platform.c has non-permissive license: proprietary-license
  • drivers/sensor/vl53l1x/vl53l1_platform.c is not apache-2.0 licensed: bsd-new
  • drivers/sensor/vl53l1x/vl53l1_platform.c is not apache-2.0 licensed: bsd-new
  • drivers/sensor/vl53l1x/vl53l1_platform.c is not apache-2.0 licensed: bsd-new
  • drivers/sensor/vl53l1x/vl53l1_platform.h is not apache-2.0 licensed: free-unknown
  • drivers/sensor/vl53l1x/vl53l1_platform.h has non-permissive license: free-unknown
  • drivers/sensor/vl53l1x/vl53l1_platform.h is not apache-2.0 licensed: proprietary-license
  • drivers/sensor/vl53l1x/vl53l1_platform.h has non-permissive license: proprietary-license
  • drivers/sensor/vl53l1x/vl53l1_platform.h is not apache-2.0 licensed: bsd-new
  • drivers/sensor/vl53l1x/vl53l1_platform.h is not apache-2.0 licensed: proprietary-license
  • drivers/sensor/vl53l1x/vl53l1_platform.h has non-permissive license: proprietary-license
  • drivers/sensor/vl53l1x/vl53l1_platform.h is not apache-2.0 licensed: bsd-new
  • drivers/sensor/vl53l1x/vl53l1_platform.h is not apache-2.0 licensed: bsd-new
  • drivers/sensor/vl53l1x/vl53l1_platform.h is not apache-2.0 licensed: bsd-new
  • drivers/sensor/vl53l1x/vl53l1_platform_log.h is not apache-2.0 licensed: free-unknown
  • drivers/sensor/vl53l1x/vl53l1_platform_log.h has non-permissive license: free-unknown
  • drivers/sensor/vl53l1x/vl53l1_platform_log.h is not apache-2.0 licensed: proprietary-license
  • drivers/sensor/vl53l1x/vl53l1_platform_log.h has non-permissive license: proprietary-license
  • drivers/sensor/vl53l1x/vl53l1_platform_log.h is not apache-2.0 licensed: bsd-new
  • drivers/sensor/vl53l1x/vl53l1_platform_log.h is not apache-2.0 licensed: proprietary-license
  • drivers/sensor/vl53l1x/vl53l1_platform_log.h has non-permissive license: proprietary-license
  • drivers/sensor/vl53l1x/vl53l1_platform_log.h is not apache-2.0 licensed: bsd-new
  • drivers/sensor/vl53l1x/vl53l1_platform_log.h is not apache-2.0 licensed: bsd-new
  • drivers/sensor/vl53l1x/vl53l1_platform_log.h is not apache-2.0 licensed: bsd-new
  • drivers/sensor/vl53l1x/vl53l1_platform_user_config.h is not apache-2.0 licensed: free-unknown
  • drivers/sensor/vl53l1x/vl53l1_platform_user_config.h has non-permissive license: free-unknown
  • drivers/sensor/vl53l1x/vl53l1_platform_user_config.h is not apache-2.0 licensed: proprietary-license
  • drivers/sensor/vl53l1x/vl53l1_platform_user_config.h has non-permissive license: proprietary-license
  • drivers/sensor/vl53l1x/vl53l1_platform_user_config.h is not apache-2.0 licensed: bsd-new
  • drivers/sensor/vl53l1x/vl53l1_platform_user_config.h is not apache-2.0 licensed: proprietary-license
  • drivers/sensor/vl53l1x/vl53l1_platform_user_config.h has non-permissive license: proprietary-license
  • drivers/sensor/vl53l1x/vl53l1_platform_user_config.h is not apache-2.0 licensed: bsd-new
  • drivers/sensor/vl53l1x/vl53l1_platform_user_config.h is not apache-2.0 licensed: bsd-new
  • drivers/sensor/vl53l1x/vl53l1_platform_user_config.h is not apache-2.0 licensed: bsd-new
  • drivers/sensor/vl53l1x/vl53l1_platform_user_data.h is not apache-2.0 licensed: free-unknown
  • drivers/sensor/vl53l1x/vl53l1_platform_user_data.h has non-permissive license: free-unknown
  • drivers/sensor/vl53l1x/vl53l1_platform_user_data.h is not apache-2.0 licensed: proprietary-license
  • drivers/sensor/vl53l1x/vl53l1_platform_user_data.h has non-permissive license: proprietary-license
  • drivers/sensor/vl53l1x/vl53l1_platform_user_data.h is not apache-2.0 licensed: bsd-new
  • drivers/sensor/vl53l1x/vl53l1_platform_user_data.h is not apache-2.0 licensed: proprietary-license
  • drivers/sensor/vl53l1x/vl53l1_platform_user_data.h has non-permissive license: proprietary-license
  • drivers/sensor/vl53l1x/vl53l1_platform_user_data.h is not apache-2.0 licensed: bsd-new
  • drivers/sensor/vl53l1x/vl53l1_platform_user_data.h is not apache-2.0 licensed: bsd-new
  • drivers/sensor/vl53l1x/vl53l1_platform_user_data.h is not apache-2.0 licensed: bsd-new
  • drivers/sensor/vl53l1x/vl53l1_platform_user_defines.h is not apache-2.0 licensed: free-unknown
  • drivers/sensor/vl53l1x/vl53l1_platform_user_defines.h has non-permissive license: free-unknown
  • drivers/sensor/vl53l1x/vl53l1_platform_user_defines.h is not apache-2.0 licensed: proprietary-license
  • drivers/sensor/vl53l1x/vl53l1_platform_user_defines.h has non-permissive license: proprietary-license
  • drivers/sensor/vl53l1x/vl53l1_platform_user_defines.h is not apache-2.0 licensed: bsd-new
  • drivers/sensor/vl53l1x/vl53l1_platform_user_defines.h is not apache-2.0 licensed: proprietary-license
  • drivers/sensor/vl53l1x/vl53l1_platform_user_defines.h has non-permissive license: proprietary-license
  • drivers/sensor/vl53l1x/vl53l1_platform_user_defines.h is not apache-2.0 licensed: bsd-new
  • drivers/sensor/vl53l1x/vl53l1_platform_user_defines.h is not apache-2.0 licensed: bsd-new
  • drivers/sensor/vl53l1x/vl53l1_platform_user_defines.h is not apache-2.0 licensed: bsd-new

erwango
erwango requested changes Aug 30, 2019
View reviewed changes
Copy link
Member

@erwango erwango left a comment •
edited
Loading

Choose a reason for hiding this comment

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

First batch of comments, thanks for this submission!
Please update west.yml manifest file to use the PR you submitted in hal_st (cf https://docs.zephyrproject.org/latest/contribute/index.html?highlight=modules#contributions-to-external-modules)
Also add your sign-off in commit message

drivers/sensor/vl53l1x/vl53l1_platform.h Outdated
Copy link
Member

@erwango erwango Aug 30, 2019

Choose a reason for hiding this comment

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

ditto

drivers/sensor/vl53l1x/vl53l1_platform_log.h Outdated
Copy link
Member

@erwango erwango Aug 30, 2019

Choose a reason for hiding this comment

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

Ditto

drivers/sensor/vl53l1x/vl53l1_platform_user_config.h Outdated
Copy link
Member

@erwango erwango Aug 30, 2019

Choose a reason for hiding this comment

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

Ditto

drivers/sensor/vl53l1x/vl53l1_platform_user_data.h Outdated
Copy link
Member

@erwango erwango Aug 30, 2019

Choose a reason for hiding this comment

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

Not sure all these files are actually needed here

Copy link
Contributor Author

@chris-schra chris-schra Aug 30, 2019

Choose a reason for hiding this comment

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

they're included in the platform-folder of STSW-IMG007 and basically I just copied everything from there to the driver folder - I think it makes sense.
I just wonder why the driver got so much more files compared to VL53L0X.
Also, atm
VL53L1_WriteMulti
is not working (as far as I can tell the other I2C funcs work).

I'll push the updated files but they will still have most of the style errors.

@chris-schra
Copy link
Contributor Author

chris-schra commented Aug 30, 2019

well, that's why I added [DNM] and [RFC] - I know that there are code changes to do, I just knew no other place to start - code via mailing list is.. urgh

@chris-schra
Copy link
Contributor Author

chris-schra commented Aug 30, 2019

what about the licensing issues in the platform files? ST gives a choice of BSD or ST proprietary but not Apache-2.0

@chris-schra chris-schra force-pushed the add-vl53l1x-driver branch 3 times, most recently from 4372172 to 9078dcb Compare August 30, 2019 14:00
@chris-schra chris-schra force-pushed the add-vl53l1x-driver branch 2 times, most recently from ade598c to 9f5a4ab Compare August 30, 2019 15:01
@chris-schra
Copy link
Contributor Author

chris-schra commented Aug 30, 2019 •
edited
Loading

What is this issue about?

1b04962be0c88d2c2705e2c78e2ce4dd2ee77c31: author email (Chris Schramm [email protected]) needs to match one of the signed-off-by entries.

I tried different combinations of the signed-off message but none does work

And what‘s about this execute permission?

@erwango
Copy link
Member

erwango commented Sep 2, 2019

@chris-makaio, for sign-off, you'll find info here: https://docs.zephyrproject.org/latest/contribute/index.html
About Licence update, I'll check internally in ST and will get back to you?

And what‘s about this execute permission?

I'm not sure what you're referring to

@erwango
Copy link
Member

erwango commented Sep 3, 2019

@chris-makaio, for ST license update to Apache 2.0, we need first to check what files are requested for this. Can you confirm the names of the files that are actually requested to be in Zephyr repo? Specially, I'd like to understand the difference with files that were used for driver vl53l1x.

@chris-schra
Copy link
Contributor Author

chris-schra commented Sep 3, 2019 •
edited
Loading

@erwango I'm confused. Aren't exactly those files listed under
#18772 (comment)
above?
The files are the boilerplate platform-specific files required to "bridge" to the ST driver in PR
zephyrproject-rtos/hal_st#2
I adopted these files to the Zephyr kernel and implemented the required functions (sleep -> k_sleep, i2c related driver functions, etc)

Concerning sign off: I think it was an old issue which already disappeared

@erwango
Copy link
Member

erwango commented Sep 3, 2019

@chris-makaio, there is already a driver for a similar sensor (vl53l0x) in zephyr tree.
For this sensor the files pushed in zephyr tree, coming from ST lib are the following:

- vl53l0x_platform.c 
- vl53l0x_platform.h
- vl53l0x_platform_log.h
- vl53l0x_types.h

I haven't check in details difference between ST lib for vl53l0x and vl53l1x, but I'd expect them to be similar. Hence I'd like to understand why the set of files you're pushing in this PR is different.

@chris-schra
drivers: sensors: add ST VL53L1X TOF sensor
d5483b1 
Adds Zephyr platform driver for VL51L1X TOF sensor,
based on ST's driver API (see UM2356)

Signed-off-by: Chris Schramm <[email protected]>
@chris-schra
Copy link
Contributor Author

chris-schra commented Sep 3, 2019

@erwango because it's a different and newer driver for a different device. The files are from ST's STSW-IMG007 and are just a boilerplate which one has to add the platform-specific implementation to. I did not want to mess with the files so I deliberately kept all files (also, for easier maintenance) and just added implementations where needed

@chris-schra chris-schra changed the title [RFC] [DNM] drivers: sensors: add ST VL53L1X TOF sensor drivers: sensors: add ST VL53L1X TOF sensor Sep 3, 2019
@chris-schra
Copy link
Contributor Author

chris-schra commented Sep 3, 2019

changed the title and removed DNM/RFC because the driver is basically complete now

@avisconti
Copy link
Contributor

avisconti commented Sep 3, 2019

@erwango @chris-makaio
I remembered now that there was a pending PR for the same component see #15566.
That PR was still using the old ext/hal path for external module.
I think it is correct to involve also @overheat in the discussion!

@avisconti avisconti mentioned this pull request Sep 3, 2019
Closed
@avisconti avisconti requested a review from erwango September 3, 2019 09:44
@chris-schra
Copy link
Contributor Author

chris-schra commented Sep 3, 2019

So any changes left for me to do?

@erwango
Copy link
Member

erwango commented Sep 5, 2019

@chris-makaio, before going on with in deep review, we need to check if it is possible to convert the files you propose to upstream in zephyr main tree to Apache 2.0 licence. I need to get this information internally from with ST. I'll get back to you on that

@erwango
Copy link
Member

erwango commented Oct 11, 2019

@chris-makaio, quick message to update you on the situation: this is progressing, I hope to have an answer next week.

@erwango
Copy link
Member

erwango commented Oct 16, 2019

@chris-makaio, I inform you that ST allows the change of License to Apache 2.0 in the files you're importing for x-cube package. Though, please keep STMicro copyright.

@erwango
Copy link
Member

erwango commented Nov 8, 2019

@chris-makaio, do you consider to complete work on this PR?

@galak galak added area: Devicetree Binding PR modifies or adds a Device Tree binding and removed area: Devicetree labels Jan 30, 2020
@galak
Copy link
Contributor

galak commented Apr 17, 2020

Closing in favor of #22747

@galak galak closed this Apr 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@avisconti avisconti Awaiting requested review from avisconti

@MaureenHelm MaureenHelm Awaiting requested review from MaureenHelm

@nashif nashif Awaiting requested review from nashif

@erwango erwango Awaiting requested review from erwango

Assignees

No one assigned

Labels

area: Devicetree Binding PR modifies or adds a Device Tree binding area: Modules area: Sensors Sensors area: Tests Issues related to a particular existing or missing test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@chris-schra @zephyrbot @erwango @avisconti @galak