west.yml: Add hal gigadevice

[nandojve]

This add modules/hal/gigadevice with three firmware libraries:

• GD32E103 ARM (Cortex-M4F)
• GD32F403 ARM (Cortex-M4F)
• GD32VF103 RISC-V (Nucleisys Bumblebee core)

Signed-off-by: Gerson Fernando Budke gerson.budke@atl-electronics.com

[github-actions]

The following west manifest projects have been modified in this Pull Request:

Name	Old Revision	New Revision	Diff
hal_gigadevice	N/A	zephyrproject-rtos/hal_gigadevice@ce323de (main)	N/A

Note: This message is automatically posted and updated by the Manifest GitHub Action.

[gmarull]

Thanks for this effort, overall LGTM. I think that HAL should be compiled in a more efficient manner, as it is done, e.g. by STM32:

# create a library and include sources as needed, e.g.
zephyr_sources(standard_peripheral/Source/gd32e10x_adc.c)
# should become
zephyr_library_sources_ifdef(CONFIG_USE_GD32_HAL_ADC standard_peripheral/Source/gd32e10x_adc.c)

[nandojve]

rebase

[tejlmand]

Some minor nits observed that you can address if you like.

Also please reply to this:
#38658 (comment)

Could you update the commit message, currently it states:

west.yml: Add gigadevice hal

This add modules/hal/gigadevice with three firmware libraries:

• GD32E10X ARM (Cortex-M4F)
• GD32F403 ARM (Cortex-M4F)
• GD32VF103 RISC-V (Nucleisys Bumblebee core)

but with your latest push, this commit does a lot more than just updating west.yml

[nandojve]

Hi @tejlmand ,

Thank you for your comments.

but with your latest push, this commit does a lot more than just updating west.yml

You are right, I split commit to distinguish both details. Since review started a lot of changes were made, including move rules from module to zephyr. If it is required, I can move rules commit to #38661.

[tejlmand]

You are right, I split commit to distinguish both details. Since review started a lot of changes were made, including move rules from module to zephyr.

Yes, this is common to happen as PR evolves.
Exactly why I usually take a second look at the commit message (those are important during bisect)

Having two commits is fine as long as the code is still bisectable, which seems to be the case in this PR, as the manifest commit is first commit.

If it is required, I can move rules commit to #38661.

No need, I think it is fine as it is.

[tejlmand]

Actually I was wrong on this, the PR is not bisectable.

If trying to bisect, the first commit fails as the glue code is not present with the new Zephyr module, so please squash the commits into a single commit..

Error seen:

Parsing /projects/github/ncs/zephyr/Kconfig
/projects/github/ncs/zephyr/scripts/kconfig/kconfig.py: build/Kconfig/Kconfig.modules:8: Could not open '/projects/github/ncs/zephyr/' (in 'osource "$(ZEPHYR_HAL_GIGADEVICE_KCONFIG)"') (EISDIR: Is a directory)
CMake Error at /projects/github/ncs/zephyr/cmake/kconfig.cmake:269 (message):
  command failed with return code: 1
Call Stack (most recent call first):
  /projects/github/ncs/zephyr/cmake/app/boilerplate.cmake:542 (include)
  /projects/github/ncs/zephyr/share/zephyr-package/cmake/ZephyrConfig.cmake:24 (include)
  /projects/github/ncs/zephyr/share/zephyr-package/cmake/ZephyrConfig.cmake:60 (include_boilerplate)
  CMakeLists.txt:5 (find_package)


-- Configuring incomplete, errors occurred!

[nandojve]

If trying to bisect, the first commit fails as the glue code is not present with the new Zephyr module, so please squash the commits into a single commit..

Done!

Below didn't pass CI because CONFIG_SOC_SERIES_GD32F403 is not yet defined.

if(CONFIG_SOC_SERIES_GD32F403)
  set(gd32_soc_uc GD32F403)
  set(gd32_soc_lc gd32f403)
endif()

I initially added only the test condition for gd32_soc_uc to ensure no one will use without proper defines. The entry should be added on each SoC series from now. It has been a lot of back and forward but result has become very good.
Thank you all for the tips!

[tejlmand]

Below didn't pass CI because CONFIG_SOC_SERIES_GD32F403 is not yet defined.

That doesn't sound right, cause in CMake CONFIG_SOC_SERIES_GD32F403 will just be an undefined var and resolve to FALSE when not available in Kconfig yet.

Something else must have been causing the error.

[nandojve]

Below didn't pass CI because CONFIG_SOC_SERIES_GD32F403 is not yet defined.

That doesn't sound right, cause in CMake CONFIG_SOC_SERIES_GD32F403 will just be an undefined var and resolve to FALSE when not available in Kconfig yet.

Something else must have been causing the error.

I probably express myself wrongly, The error was just a compliance check, it not occurred at buildkite. When I told CI, I mean not 100% green on all checks.

[nandojve]

Hi @tejlmand ,

It seems everything is green again. I checked that library is Ok with current configs.
Can you revisit and look at CMakeLists.txt ?

[tejlmand]

I probably express myself wrongly, The error was just a compliance check, it not occurred at buildkite. When I told CI, I mean not 100% green on all checks.

yep, I see the error in the compliance check.

In that case, I would actually recommend that this PR becomes part of #38661
Of course it's always nice if PRs with multiple commits can be split into independent pieces, but it seems that this PR and #38661 are closely coupled.

[nandojve]

I probably express myself wrongly, The error was just a compliance check, it not occurred at buildkite. When I told CI, I mean not 100% green on all checks.

yep, I see the error in the compliance check.

In that case, I would actually recommend that this PR becomes part of #38661 Of course it's always nice if PRs with multiple commits can be split into independent pieces, but it seems that this PR and #38661 are closely coupled.

After a lot of changes a more robust rules for this SoC were achieved. I afraid that if we join both PRs we may loose test conditions. For instance, The Fatal condition has been checked with/without CONFIG_SOC_SERIES_GD32xxx and passes on both cases. Since we are testing this PR and #38661 we cover both situations.

[nandojve]

I agreed that this PR became essential part of #38661. We already covered the edge cases. I'll close here and keep it only at #38661.
Thank you for all contributions.