-
Couldn't load subscription status.
- Fork 8.1k
[RFC] ST sensor drivers migration to STdC #14111
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
[RFC] ST sensor drivers migration to STdC #14111
Conversation
|
Found the following issues, please fix and resubmit: License issuesIn most cases you do not need to do anything here, especially if the files
|
2a83d6c to
992840e
Compare
|
@erwango |
|
@MaureenHelm |
992840e to
5d885d7
Compare
5d885d7 to
7f1c632
Compare
|
@erwango @MaureenHelm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few comments on packages integration, otherwise looks good.
ext/hal/st/stmemsc/README
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this Licence agreement is raising concerns.
I don't know how compatible is the use of SLA with Zephyr policy. As far as I understand, using this package binds to agree these terms, not sure how it fits with Apache 2.0 (is that strictly orthogonal)?
Is that SLA strictly needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have the same agreement cited in all stm32cube packages, like ext/hal/st/stm32cube/stm32f4xx/README for example.
Why should be an issue here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, let me sort this out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@avisconti , I'm updating the link for Cube packages: #15731
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@avisconti , I'm updating the link for Cube packages: #15731
OK, I'm going to change it as well. Thx!
ext/hal/st/stmemsc/README.md
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this file be kept as is?
As I understand you stripped from the initial package structure and some parts of this README aren't valid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct! I removed all the examples. So I will strip off from the README.md all references to examples.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, should we keep those?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmmmh ok.
So I see two options now. Either keeping also the examples in the package or adapting all the README (but there are many) stripping of all references to examples.
@erwango
If you don't see issues I may go with option one which seems to be less impacting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure adding examples juts because README files refer to it is a great option.
Do we really need these README files at all?
I don't see much added value in the information provided, this is only copy/paste from one to the other, and most of the information is not valid as referring to examples that you initially considered as superfluous (and I think you were right).
The only interesting information that differ from one file to the other is H3LIS331DL DS rev3.0 , which could be factorized in a upper file if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, your preferred option would be:
- Package w/o examples
- Remove all README files under drivers
- Keep the top README file w/o reference to example
Correct?
I'm ok with it. The only thing is that everytime I have to re-align the package there is an
extra effort (but not that much) to clean the package up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is indeed indeed my preferred way to do it.
For the extra pain to do it, I suggest to spend few hours on a script to do it for you.
I have this for cube packages update and this saves me painful hours each time.
7f1c632 to
a3b9082
Compare
|
rebased and repushed as agreed with @erwango |
|
@erwango |
31fe99b to
91d7221
Compare
|
I got now that I have to solve a couple of false postives in UNDEF_KCONFIG_WHITELIST. |
@avisconti, as discussed, can you add your name in front of stmemsc in CODEOWNER? |
Yes I did. Now I made a change in the ci-tools whitelist in order to have the CI passing for this PR. |
It's perfect, much more than my monday eyes. |
c79dfaa to
0f61a46
Compare
|
@erwango @galak CONFIG_NEWLIB_LIBC=y Before I used 'select' directly in stmemsc Kconfig file, but I saw that usage of 'select' for visible
So, can you help me in using the right approach for fixing CI? EDIT: EDIT2: |
So I think you should have a separate test for this if newlib is required for the sensor drivers. We shouldn't burden sensors_i_z.conf with that requirement. If that makes its easier to use 'depends' than great. However, I'd try 'imply' and see how that works. |
8d3d81b to
5e840d0
Compare
|
re-pushed again |
|
@erwango @MaureenHelm I think the code now is in a good shape, isn't it? |
|
@avisconti , seems there was an issue in CODEOWNER file that prevented correct parsing. |
5e840d0 to
86c0474
Compare
OK, thx! |
|
@MaureenHelm |
Added:
CONFIG_LSM6DSO_INT_PIN
CONFIG_PEDO_THS_MIN
CONFIG_USE_STDC_
Referenced in PR zephyrproject-rtos#14111 and PR zephyrproject-rtos#15425.
Signed-off-by: Armando Visconti <armando.visconti@st.com>
|
@galak @MaureenHelm |
|
@erwango |
|
@avisconti , BSD-3 Clause is fine, sure. |
|
@nashif, do you want to merge this one now? or latter once the modules are there? |
This package contains platform independent drivers written in C language for STMicroelectronics sensors. The aim of this package is to provide a common, clean and stable interface to access sensor registers. Library is located in ext/hal/st/stmemsc/ Origin: ST Microelectronics License: BSD-3-Clause URL: https://www.st.com/en/embedded-software/c-driver-mems.html Commit: v1.00 Purpose: provide a common and stable i/f to access sensor registers Maintained-by: ST Microelectronics Signed-off-by: Armando Visconti <armando.visconti@st.com>
Port the lis2dw12 sensor driver on top of the lis2dw12_StdC HAL interface (under ext/hal/st/stmemsc/). Signed-off-by: Armando Visconti <armando.visconti@st.com>
86c0474 to
dd087be
Compare
|
@nashif @erwango
|
|
@nashif @erwango |
Added:
CONFIG_LSM6DSO_INT_PIN
CONFIG_PEDO_THS_MIN
CONFIG_USE_STDC_
Referenced in PR zephyrproject-rtos#14111 and PR zephyrproject-rtos#15425.
Signed-off-by: Armando Visconti <armando.visconti@st.com>
STdC is a standard package written by ST that aims to provide a standard register i/f for ST MEMS
sensors. It is already offered/used in other s/w systems as well.
The idea behind it is to have a clean and stable functions/defines to read/write sensors registers.
Official public repo is:
https://github.com/STMicroelectronics/STMems_Standard_C_drivers
The license seems to be the same as stm32cube, so there should be no issues here.
Since STdC package is including math.h I had to enable NEWLIB_LIBC.
In this RFC I did an experimental test using lis2dw12 driver. It works for both i2c and spi.
Please provide your feedback.