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

kinetis: move filtering-out periph_hwrng in cpu/kinetis #11479

Merged
merged 1 commit into from
May 6, 2019

Conversation

cladmi
Copy link
Contributor

@cladmi cladmi commented May 2, 2019

Contribution description

This removes doing filter-out periph_hwrng, $(FEATURES_PROVIDED)
after processing cpu/$(CPU)/Makefile.features.
The current solution is a HACK as CPU_MODEL is currently not available
at that moment but will be in the near future.

It will allow always including cpu/$(CPU)/Makefile.features after
boards/$(BOARD)/Makefile.features.

It is a part of moving CPU/CPU_MODEL definitions to Makefile.features.

Testing procedure

For all the modified boards, the provided features are the same as in master.
I only checked examples/hello-world as it should not change anything for these ones.

for board in frdm-k64f frdm-kw41z phynode-kw41z teensy31 usb-kw41z; do echo ${board}; BOARD=${board} make --no-print-directory -C examples/hello-world info-debug-variable-FEATURES_PROVIDED; done
frdm-k64f
periph_adc periph_i2c periph_pwm periph_rtc periph_rtt periph_spi periph_timer periph_uart periph_cpuid periph_gpio periph_gpio_irq periph_mcg periph_pm cpp
frdm-kw41z
periph_adc periph_i2c periph_rtc periph_rtt periph_spi periph_timer periph_uart periph_cpuid periph_gpio periph_gpio_irq periph_mcg periph_pm cpp
phynode-kw41z
periph_adc periph_i2c periph_rtc periph_rtt periph_spi periph_timer periph_uart periph_cpuid periph_gpio periph_gpio_irq periph_mcg periph_pm cpp
teensy31
periph_pwm periph_rtc periph_rtt periph_timer periph_uart periph_cpuid periph_gpio periph_gpio_irq periph_mcg periph_pm cpp
usb-kw41z
periph_adc periph_i2c periph_rtc periph_rtt periph_spi periph_timer periph_uart periph_cpuid periph_gpio periph_gpio_irq periph_mcg periph_pm cpp

I also looked at all boards to verify they did not change but did not post the output.

Issues/PRs references

Required to apply #11478
Split out of #11477

@cladmi cladmi added Area: build system Area: Build system CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Area: boards Area: Board ports labels May 2, 2019
@cladmi cladmi added this to the Release 2019.07 milestone May 2, 2019
@cladmi cladmi requested review from aabadie and fjmolinas May 2, 2019 16:39
Copy link
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

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

ACK for the proposed HACK. I would just reword a bit the comments.

FEATURES_PROVIDED += periph_hwrng

# HACK Do not define 'hwrng' if the board does not supports it
# I prefer a whitelist on CPU_MODEL but the information is not available yet
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment has too much personal taste (e.g. don't use I prefer). Maybe reword with: A whitelist on CPU_MODEL would be better but this information/variable is not available yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, will change.


# HACK Do not define 'hwrng' if the board does not supports it
# I prefer a whitelist on CPU_MODEL but the information is not available yet
# HACK the board/cpu_model currently uses the wrong hwrng register
Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand this correctly, the wrong hwrng register is used for frdm-k64f ? Maybe make this more explicit that this HACK applies to frdm-kw64f.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I forgot to re-precise this when moving it to the common directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While re-reading I am not sure if it is the frdm-k64f or the CPU_MODEL completely. That is why I wrote board/cpu_model. I think it may be more cpu_model but hard to know without debugging why.

I can change to Hack, either the board or the cpu_model currently uses the wrong hwrng maybe ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe start the next sentence on the same line:
HACK the board/cpu_model currently uses the wrong hwrng register. Remove next line when fixed (you can split it if it's too long)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can just remove the "remove next line when fixed" as it is implicit and still precise the frdm as it is currently the only one I know as you said in the first comment.
The HWRNG uses the wrong hwrng register for the frdm-k64f board/cpu_model

Copy link
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

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

Looks good now. ACK

Please squash

This removes doing `filter-out periph_hwrng, $(FEATURES_PROVIDED)`
after processing `cpu/$(CPU)/Makefile.features`.
The current solution is a HACK as `CPU_MODEL` is currently not available
at that moment but will be in the near future.

It will allow always including `cpu/$(CPU)/Makefile.features` after
`boards/$(BOARD)/Makefile.features`.

It is a part of moving `CPU/CPU_MODEL` definitions to `Makefile.features`.
@cladmi cladmi force-pushed the pr/kinetis/hwrng branch from 8676f2e to 19224ec Compare May 6, 2019 13:13
@cladmi
Copy link
Contributor Author

cladmi commented May 6, 2019

Squashed.

@aabadie
Copy link
Contributor

aabadie commented May 6, 2019

and go!

@aabadie aabadie merged commit 990086a into RIOT-OS:master May 6, 2019
@cladmi
Copy link
Contributor Author

cladmi commented May 6, 2019

Thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: boards Area: Board ports Area: build system Area: Build system CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants