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

chore(Other): Add zephyr related wrapper and system file in MSDK #990

Merged
merged 3 commits into from
Apr 16, 2024

Conversation

ozersa
Copy link
Contributor

@ozersa ozersa commented Apr 15, 2024

Description

hal_adi is repository that used by zephyr sdk.
By this PR the content of this repository moved in MSDK repository to centralize it.

  • Add wrappers files that used to provide common interface on zephyr shim driver.
  • Add CMakefies that used to build drivers
  • Update zephyr-hal.sh script to copy new files to target repository (hal_adi).

Wrapper layer added to get one common zephyr shim driver on zephyr layer.
image

It will be nice if wrap layer function been added peripheral driver in future.

hal_adi folder layout listed below. After this PR being merged MAX folder content totally will comes from MSDK.
image

Checklist Before Requesting Review

  • PR Title follows correct guidelines.
  • Description of changes and all other relevant information.
  • (Optional) Link any related GitHub issues using a keyword
  • (Optional) Provide info on any relevant functional testing/validation. For API changes or significant features, this is not optional.

@github-actions github-actions bot added the Workflow Related to Workflow development label Apr 15, 2024
@ozersa ozersa force-pushed the add-zephyr-files branch from 7a8904f to e98ae4c Compare April 15, 2024 10:43
@@ -0,0 +1,171 @@
# Makefile - Analog Devices SDK
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the copyright/license text in these CMakeLists.txt files match the copyrights used in the other files? Or do these files need the particular format you used?

Copy link
Contributor

Choose a reason for hiding this comment

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

@sihyung-maxim, what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this copyright/license text still works because the license identifier references Apache 2.0, and our LICENSE file at the root of the MSDK repo applies to every file and subdirectory within the root.

.txt files don't need to follow the same format as the source files as it depends on the context of how they're used - they can be used for documentation, data storing, configuration setting, and even as scripts/code. I think it's more of a matter of whether we want to keep consistency with our files, but I recommend using our default copyrights in this case since it's used in a scripting/make context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was default copyright lines that we and other vendor added in zephyr files.
But it make sense it be consistent on the hal layer.
Updated please see: 632d579

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree, be consistent with the rest of the MSDK

@ozersa ozersa force-pushed the add-zephyr-files branch from 4f85fea to cd88f6a Compare April 16, 2024 09:31
ozersa added 3 commits April 16, 2024 16:48
hal_adi content moved insdie MSDK as it is

Signed-off-by: Sadik Ozer <[email protected]>
Signed-off-by: Sadik Ozer <[email protected]>
Copyright section uptaed to it be consistent
Add missing (C)

Signed-off-by: Sadik Ozer <[email protected]>
@ozersa ozersa force-pushed the add-zephyr-files branch from cd88f6a to db92826 Compare April 16, 2024 13:52
@lorne-maxim lorne-maxim merged commit 790ad99 into main Apr 16, 2024
10 checks passed
@lorne-maxim lorne-maxim deleted the add-zephyr-files branch April 16, 2024 15:28
Copy link
Contributor

Choose a reason for hiding this comment

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

@ozersa @MaureenHelm FYI we have #include <mxc_device.h> for each micro that we can use here to make maintenance of this file a bit easier. It does the same job

(ex https://github.com/analogdevicesinc/msdk/blob/main/Libraries/PeriphDrivers/Include/MAX78000/mxc_device.h)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, due to we need to add extra addition as below in past we added a new files, now this file might be replaced with mxc_device.h.
image

Copy link
Contributor

Choose a reason for hiding this comment

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

@ozersa does this add the source files to the top-level build? Since Zephyr uses CMake it would also be nice to start setting things up more modularly for each library to support find_package

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks noted.

endif()
string(TOUPPER ${TARGET_LC} TARGET_UC)

zephyr_compile_definitions(
Copy link
Contributor

Choose a reason for hiding this comment

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

@ozersa also suggest adding -D${TARGET_UC}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, will consider on next update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Jake-Carter I took a look it now, to apply it on #996
But a little confused.
Do you propose replacing
-DTARGET=${TARGET_UC} with -DTARGET=-D${TARGET_UC} ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@ozersa just to have both TARGET and the actual value of TARGET_UC defined.

zephyr_compile_definitions(
    -D${TARGET_UC}
    -DTARGET=${TARGET_UC}
    ...
)

Some places in the drivers need to check the target value. Since the pre-processor can't do string comparisons sometimes TARGET_UC itself is checked.

#ifdef MAX78000
// ...
#endif

EricB-ADI pushed a commit that referenced this pull request Aug 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Workflow Related to Workflow development
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants