Skip to content

Remove dependency on us_ticker HAL apis for non USTICKER targets #10155

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

Merged
merged 4 commits into from
Mar 27, 2019
Merged

Remove dependency on us_ticker HAL apis for non USTICKER targets #10155

merged 4 commits into from
Mar 27, 2019

Conversation

mikisch81
Copy link
Contributor

@mikisch81 mikisch81 commented Mar 19, 2019

Description

During #9221 there were difficulties building a secure image without USTICKER and as a workaround a dummy implementation of us_ticker HAL apis was built in the secure image.

#9896 handled part of the problem by calling wait_ns() in wait APIs when there is no USTICKER, but still mbed_us_ticker_api.c defined a static struct of function pointers pointing to the usticker hal APIs and there are multiple places in the tree which call get_us_ticker_data().

So this PR solves these issues by:

  • Surround mbed_us_ticker_api.c with if DEVICE_USTICKER.
  • get_us_ticker_data() returns NULL for non-usticker targets.

Fixes #10139

Pull request type

[X] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@ARMmbed/mbed-os-psa @kjbracey-arm

Release Notes

@mikisch81
Copy link
Contributor Author

mikisch81 commented Mar 20, 2019

Added USTICKER to ARCH_PRO target, according to 10139#issuecomment if this target uses usticker_api.c it should define DEVICE_USTICKER.
@kjbracey-arm @ARMmbed/team-seeed @maclobdell

@mikisch81
Copy link
Contributor Author

@cmonr Can we start CI just to check if ARCH_PRO builds and run ok after last commit?

@mikisch81
Copy link
Contributor Author

Of course pending @ARMmbed/team-seeed review

@cmonr
Copy link
Contributor

cmonr commented Mar 20, 2019

CI started

@mbed-ci
Copy link

mbed-ci commented Mar 20, 2019

Test run: FAILED

Summary: 3 of 9 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-GCC_ARM
  • jenkins-ci/mbed-os-ci_build-IAR8
  • jenkins-ci/mbed-os-ci_build-ARMC6

@0xc0170
Copy link
Contributor

0xc0170 commented Mar 21, 2019

Please review build failures (related to the changes)

@0xc0170 0xc0170 requested a review from a team March 21, 2019 12:18
@0xc0170
Copy link
Contributor

0xc0170 commented Mar 21, 2019

The failures point to undefined functions from us ticker - looks like their target code is not protected, please review

@mikisch81
Copy link
Contributor Author

Added USTICKER to these targets:

  • ARCH_PRO
  • LPC4088
  • LPC4088_DM
  • MAX32600MBED
  • NCS36510
  • WIZWIKI_W7500
  • WIZWIKI_W7500ECO
  • WIZWIKI_W7500P

All of these targets had us_ticker.c in their compilation but didn't have "device_has": "USTICKER" in target.json

@ARMmbed/team-nxp @ARMmbed/team-seeed please review changes to your targets

@ARMmbed/mbed-os-maintainers @maclobdell can you add more partners according to the affected targets?

@mikisch81
Copy link
Contributor Author

@cmonr can we run CI to check if last commit helped?

@0xc0170 0xc0170 requested review from a team March 22, 2019 05:46
@0xc0170
Copy link
Contributor

0xc0170 commented Mar 22, 2019

All of these targets had us_ticker.c in their compilation but didn't have "device_has": "USTICKER" in target.json

👍 I would not expect to have this not being defined.

We will start CI later today, when rc4 jobs go through

@mikisch81
Copy link
Contributor Author

@ARMmbed/team-seeed @ARMmbed/team-nxp ping

@NirSonnenschein
Copy link
Contributor

Started CI

@mbed-ci
Copy link

mbed-ci commented Mar 24, 2019

Test run: FAILED

Summary: 3 of 9 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-IAR8
  • jenkins-ci/mbed-os-ci_build-GCC_ARM
  • jenkins-ci/mbed-os-ci_build-ARMC6

@mikisch81
Copy link
Contributor Author

@cmonr @NirSonnenschein @0xc0170 Why does astyle suddently looks into files inside targets/?

@mikisch81
Copy link
Contributor Author

@cmonr @NirSonnenschein @0xc0170 Why does astyle suddently looks into files inside targets/?

@cmonr @NirSonnenschein @0xc0170 can we start CI while the astyle issue is being queried?

@mikisch81
Copy link
Contributor Author

@mmahadevan108 Can you review last commit?

@NirSonnenschein
Copy link
Contributor

restarting CI to verify latest fix pending a-style issue fix

@mbed-ci
Copy link

mbed-ci commented Mar 25, 2019

Test run: FAILED

Summary: 2 of 13 test jobs failed
Build number : 3
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_dynamic-memory-usage
  • jenkins-ci/mbed-os-ci_exporter

@alekla01
Copy link
Contributor

restarted failed jenkins jobs

@mikisch81
Copy link
Contributor Author

@mmahadevan108 Can you review last commit?

@mmahadevan108

@mikisch81
Copy link
Contributor Author

@ARMmbed/mbed-os-maintainers Any updates regarding the astyle issue?

@mikisch81
Copy link
Contributor Author

mikisch81 commented Mar 25, 2019

@ARMmbed/mbed-os-maintainers Is this PR ready for merge once the astyle issue is handled?

@cmonr
Copy link
Contributor

cmonr commented Mar 25, 2019

@mikisch81 I believe so.

Bringing in the fix here: #10221

I think once the PR is merged, kicking off Travis CI again for this PR should make everything green (I suspect after the merge, the ref/pull/10155/merge reference is regenerated). If not, we can restart CI post-haste.

@mikisch81
Copy link
Contributor Author

@ARMmbed/team-nxp can you look at the last commit?

@cmonr
Copy link
Contributor

cmonr commented Mar 26, 2019

Local testing with the last astyle missed a single character. Should be good once #10222 comes in.

Michael Schwarcz and others added 4 commits March 26, 2019 09:52
- Surround mbed_us_ticker_api.c with if DEVICE_USTICKER
- get_us_ticker_data() returns NULL for non-usticker targets.
- LPC4088
- LPC4088_DM
- MAX32600MBED
- NCS36510
- WIZWIKI_W7500
- WIZWIKI_W7500ECO
- WIZWIKI_W7500P
@mikisch81
Copy link
Contributor Author

Rebased after #10222 was merged

@NirSonnenschein
Copy link
Contributor

restarting CI after rebase for astyle fix

@mbed-ci
Copy link

mbed-ci commented Mar 26, 2019

Test run: SUCCESS

Summary: 13 of 13 test jobs passed
Build number : 4
Build artifacts

@mikisch81
Copy link
Contributor Author

@ARMmbed/mbed-os-maintainers Any more stuff is needed for this PR?

@NirSonnenschein
Copy link
Contributor

@ARMmbed/team-seeed would you like to review this before it is merged? please review or indicate that you don't intend to.

cmonr pushed a commit to cmonr/mbed-os that referenced this pull request Mar 26, 2019
Remove dependency on us_ticker HAL apis for non USTICKER targets
@cmonr cmonr merged commit fcf4999 into ARMmbed:master Mar 27, 2019
@mikisch81 mikisch81 deleted the us_ticker branch March 27, 2019 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants