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

CMSIS v2 emulation assumes ticks == milliseconds #16902

Closed
andyross opened this issue Jun 18, 2019 · 7 comments
Closed

CMSIS v2 emulation assumes ticks == milliseconds #16902

andyross opened this issue Jun 18, 2019 · 7 comments
Assignees
Labels
bug The issue is a bug, or the PR is fixing a bug priority: medium Medium impact/importance bug

Comments

@andyross
Copy link
Contributor

ARM docs say that the timeout values used in CMSIS v2 are milliseconds, but our implementation (c.f. osDelay() in lib/cmsis_rtos_v2/kernel.c and other places) wants to treat these numbers as Zephyr ticks instead. Our test code seems to be using a mix of conventions.

I added a check to force the tick rate to 1000 when CMSIS is enabled as a workaround[1], but this needs to be cleaned up at some point.

[1] A fairly reasonable one. Ported CMSIS apps are obviously going to be OK with millisecond timing given that's what the API gave them to begin with.

@andyross andyross added the bug The issue is a bug, or the PR is fixing a bug label Jun 18, 2019
@andyross
Copy link
Contributor Author

Found while cleaning tests up for high tick rates in #16782

@ioannisg ioannisg added the priority: medium Medium impact/importance bug label Jun 18, 2019
@nashif nashif assigned jocelyn-li and unassigned nashif Jun 19, 2019
@nashif
Copy link
Member

nashif commented Jun 19, 2019

@jocelyn-li please assign

@jocelyn-li jocelyn-li assigned LeiW000 and unassigned jocelyn-li Aug 28, 2019
@rljordan-zz
Copy link

This is just code cleanup. Priority will be reduced to Low.

@rljordan-zz rljordan-zz added priority: low Low impact/importance bug and removed priority: medium Medium impact/importance bug labels Aug 30, 2019
@andyross
Copy link
Contributor Author

andyross commented Sep 3, 2019

FWIW: this isn't cleanup. Code using CMSISv2 timing APIs only works correctly if CONFIG_SYS_CLOCK_TICKS_PER_SEC is equal to 1000. That's not high priority, and in fact this constraint is enforced by code, but it's a real bug.

@LeiW000
Copy link
Collaborator

LeiW000 commented Sep 4, 2019

@andyross , sorry, I misunderstood it. Anyway, Peng and I will try to fix it.

@carlescufi
Copy link
Member

carlescufi commented Apr 30, 2020

@nashif @chen-png any chance we can address this?

@andrewboie andrewboie added priority: medium Medium impact/importance bug and removed priority: low Low impact/importance bug labels Apr 30, 2020
@nashif nashif assigned andyross and unassigned nashif, LeiW000 and chen-png May 4, 2020
@andyross
Copy link
Contributor Author

andyross commented May 4, 2020

Actually this was a mistaken bug to begin with. While CMSIS version one used milliseconds as the native API, version two passes system ticks to the same functions with the same names. Our code was correct to begin with, though the naming was confusing in a few places (probably owing to cut-n-paste from the v1 original).

Regardless, this is all cleaned up in #24956, which uses our now-native support for tick-based timeouts.

@andyross andyross closed this as completed May 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug The issue is a bug, or the PR is fixing a bug priority: medium Medium impact/importance bug
Projects
None yet
Development

No branches or pull requests

9 participants