Skip to content
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

Integrate Würth Elektronik Sensors SDK code for use in sensor drivers #47021

Closed
yath-eiSmart opened this issue Jun 29, 2022 · 23 comments
Closed
Labels
area: Sensors Sensors RFC Request For Comments: want input from the community

Comments

@yath-eiSmart
Copy link
Contributor

yath-eiSmart commented Jun 29, 2022

Origin

WE Sensors SDK
https://github.com/WurthElektronik/Sensors-SDK_STM32

Purpose

Drivers for sensors by Würth Elektronik.

Mode of integration

We would like to integrate the required code as a module.
Suggested module name: "hal_we".

Maintainership

@mah-eiSmart

Pull Request

#46920

Description

The aforementioned pull request adds drivers and examples for Würth Elektronik sensors to Zephyr. The drivers internally use the Würth Elektronik Sensors SDK code, which we plan to integrate as a module (the pull request currently has the SDK code integrated into Zephyr's main tree, this will be moved to a module).

Dependencies

None

Revision

Based on the next SDK version to be released (2.2).

License

Apache-2.0

@yath-eiSmart yath-eiSmart added the TSC Topics that need TSC discussion label Jun 29, 2022
@carlescufi
Copy link
Member

I am a bit concerned about future updates if we integrate this in-tree. Every time you need to update to a more recent version of your SDK you will have to reformat the code to fit Zephyr's coding style and guidelines?

@carlescufi
Copy link
Member

Also you apparently skipped the section on maintainership here: https://github.com/zephyrproject-rtos/zephyr/blob/main/.github/ISSUE_TEMPLATE/ext-source.md

Could you please add this section?

@yath-eiSmart
Copy link
Contributor Author

I added the section on maintainership.

I am a bit concerned about future updates if we integrate this in-tree. Every time you need to update to a more recent version of your SDK you will have to reformat the code to fit Zephyr's coding style and guidelines?

Actually, we intend to reformat the existing SDK according to Zephyr's requirements and keep it that way. So that should not be an issue.

@carlescufi
Copy link
Member

I added the section on maintainership.

I am a bit concerned about future updates if we integrate this in-tree. Every time you need to update to a more recent version of your SDK you will have to reformat the code to fit Zephyr's coding style and guidelines?

Actually, we intend to reformat the existing SDK according to Zephyr's requirements and keep it that way. So that should not be an issue.

OK. Unfortunately the agenda for this week's TSC meeting was sent yesterday, so this will need to be discussed in next week's meeting.

@yath-eiSmart
Copy link
Contributor Author

Ok, thanks for the info.

@yath-eiSmart
Copy link
Contributor Author

We have discussed the matter internally and decided to go with integration as a module after all, as this is probably the cleanest solution. I have changed the issue text accordingly.

@carlescufi
Copy link
Member

We have discussed the matter internally and decided to go with integration as a module after all, as this is probably the cleanest solution. I have changed the issue text accordingly.

OK, very well. I will make sure that the TSC is aware of this change.

@MaureenHelm
Copy link
Member

The license does not look like Apache 2.0.

https://github.com/WurthElektronik/Sensors-SDK_STM32/blob/main/SensorsSDK/WSEN_HIDS_2523020210001/WSEN_HIDS_2523020210001.c:

/*
 ***************************************************************************************************
 * This file is part of Sensors SDK:
 * https://www.we-online.com/sensors, https://github.com/WurthElektronik/Sensors-SDK_STM32
 *
 * THE SOFTWARE INCLUDING THE SOURCE CODE IS PROVIDED “AS IS”. YOU ACKNOWLEDGE THAT WÜRTH ELEKTRONIK
 * EISOS MAKES NO REPRESENTATIONS AND WARRANTIES OF ANY KIND RELATED TO, BUT NOT LIMITED
 * TO THE NON-INFRINGEMENT OF THIRD PARTIES’ INTELLECTUAL PROPERTY RIGHTS OR THE
 * MERCHANTABILITY OR FITNESS FOR YOUR INTENDED PURPOSE OR USAGE. WÜRTH ELEKTRONIK EISOS DOES NOT
 * WARRANT OR REPRESENT THAT ANY LICENSE, EITHER EXPRESS OR IMPLIED, IS GRANTED UNDER ANY PATENT
 * RIGHT, COPYRIGHT, MASK WORK RIGHT, OR OTHER INTELLECTUAL PROPERTY RIGHT RELATING TO ANY
 * COMBINATION, MACHINE, OR PROCESS IN WHICH THE PRODUCT IS USED. INFORMATION PUBLISHED BY
 * WÜRTH ELEKTRONIK EISOS REGARDING THIRD-PARTY PRODUCTS OR SERVICES DOES NOT CONSTITUTE A LICENSE
 * FROM WÜRTH ELEKTRONIK EISOS TO USE SUCH PRODUCTS OR SERVICES OR A WARRANTY OR ENDORSEMENT
 * THEREOF
 *
 * THIS SOURCE CODE IS PROTECTED BY A LICENSE.
 * FOR MORE INFORMATION PLEASE CAREFULLY READ THE LICENSE AGREEMENT FILE (license_terms_wsen_sdk.pdf)
 * LOCATED IN THE ROOT DIRECTORY OF THIS DRIVER PACKAGE.
 *
 * COPYRIGHT (c) 2022 Würth Elektronik eiSos GmbH & Co. KG
 *
 ***************************************************************************************************
 */

There's also this in the license file:

You are not entitled to transfer the source code in any form to third parties without prior written consent of Würth Elektronik eiSos.

@carlescufi
Copy link
Member

@yath-eiSmart you will need to fix the license. It doesn't have to be Apache v2, but it has to be OSI-approved and non-copyleft. Typical alternatives to Apache v2 are:

  • 3-clause BSD
  • MIT

@yath-eiSmart
Copy link
Contributor Author

Thank you for the feedback.

We intend to release the module using the Apache 2.0 license.

@carlescufi
Copy link
Member

@yath-eiSmart this was discussed at the TSC meeting today, and there was a request to see the future module repo in some shape or form, including:

  • All files and folders that will be there
  • Those files under the right license (Apache v2)

So we would ask you to create a repo under whatever GitHub organization you want (does not need to be the official Würth one, can be your own user) that reflects the exact contents of the future module.

Thanks for your patience!

@yath-eiSmart
Copy link
Contributor Author

@carlescufi thank you for the update.

We have created a repo for the new module as requested: https://github.com/WE-eiSmart/zephyr_hal_we

@carlescufi
Copy link
Member

@carlescufi thank you for the update.

We have created a repo for the new module as requested: https://github.com/WE-eiSmart/zephyr_hal_we

This looks great, thank you!

@yath-eiSmart
Copy link
Contributor Author

Thank you for the feedback! What are the next steps? Has this been discussed by the TSC?

We have prepared the drivers for using the external SDK module, so we are ready to proceed with the contribution.

@MaureenHelm
Copy link
Member

Thank you for the feedback! What are the next steps? Has this been discussed by the TSC?

Thank you for addressing the license issue so quickly; agree with @carlescufi it looks good now. The TSC plans to make a decision next week.

We have prepared the drivers for using the external SDK module, so we are ready to proceed with the contribution.

You can go ahead and send a draft PR that points west.yml to your module repo. We won't be able to merge it until after the TSC approves and creates a mirror in the zephyrproject-rtos org, but we can start code reviews on the driver shims in the meantime. Please don't send all the driver shims in one PR as that is difficult to review. The code review process will go more smoothly if you send one driver shim per PR. Thank you!

@yath-eiSmart
Copy link
Contributor Author

Perfect, thanks for the update!

@carlescufi
Copy link
Member

carlescufi commented Jul 20, 2022

The TSC approved this today. The repository will be created shortly.

@nashif nashif removed the TSC Topics that need TSC discussion label Jul 27, 2022
@fabiobaltieri fabiobaltieri added the RFC Request For Comments: want input from the community label Aug 9, 2022
@carlescufi
Copy link
Member

@yath-eiSmart what should the repo be called? hal_we or hal_wuerth ?

@mah-eiSmart
Copy link
Contributor

@yath-eiSmart what should the repo be called? hal_we or hal_wuerth ?

Hey @carlescufi ,
@yath-eiSmart is out of office for a short while. Thus I can tell you that "hal_we" is the right name for the repo. Thanks!

BR Matthias

@carlescufi
Copy link
Member

@yath-eiSmart what should the repo be called? hal_we or hal_wuerth ?

Hey @carlescufi , @yath-eiSmart is out of office for a short while. Thus I can tell you that "hal_we" is the right name for the repo. Thanks!

BR Matthias

Thanks @mah-eiSmart. I brought this up in today's TSC and hal_we is regarded as ambiguous, so it'd be great if you could suggest something a bit more specific, like hal_wuerth or hal_wurthelek or something like that.

@mah-eiSmart
Copy link
Contributor

Hello @carlescufi ,
ok, got it. Please check if hal_wurthelektronik is possible, if not, take hal_wurth.
Thanks for support!
BR, Matthias

@carlescufi
Copy link
Member

@yath-eiSmart
Copy link
Contributor Author

Repo created: https://github.com/zephyrproject-rtos/hal_wurthelektronik

@carlescufi Great, thanks! Unfortunately, I currently don't have write access to the repo - I was out of office and the invitation probably expired. Could you reinvite me? Thanks!

fabiobaltieri added a commit to fabiobaltieri/zephyr that referenced this issue Sep 20, 2022
Adding mah-eiSmart as maintainer for hal_wurthelektronik.

Link: zephyrproject-rtos#47021 (comment)
Signed-off-by: Fabio Baltieri <fabiobaltieri@google.com>
fabiobaltieri added a commit that referenced this issue Sep 20, 2022
Adding mah-eiSmart as maintainer for hal_wurthelektronik.

Link: #47021 (comment)
Signed-off-by: Fabio Baltieri <fabiobaltieri@google.com>
coreboot-org-bot pushed a commit to coreboot/zephyr-cros that referenced this issue Sep 21, 2022
Adding mah-eiSmart as maintainer for hal_wurthelektronik.

(cherry picked from commit 6030b85)

Original-Link: zephyrproject-rtos/zephyr#47021 (comment)
Original-Signed-off-by: Fabio Baltieri <fabiobaltieri@google.com>
GitOrigin-RevId: 6030b85
Change-Id: I3de050a13fc3888c9e6f1f85f869de4774115046
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/zephyr/+/3906298
Reviewed-by: Fabio Baltieri <fabiobaltieri@google.com>
Commit-Queue: Fabio Baltieri <fabiobaltieri@google.com>
Tested-by: Fabio Baltieri <fabiobaltieri@google.com>
Tested-by: CopyBot Service Account <copybot.service@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Sensors Sensors RFC Request For Comments: want input from the community
Projects
None yet
Development

No branches or pull requests

7 participants