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

Timer "reconfig" tests do not work on RTEMS or VxWorks #1083

Closed
jphickey opened this issue Jun 25, 2021 · 5 comments · Fixed by #1089 or #1100
Closed

Timer "reconfig" tests do not work on RTEMS or VxWorks #1083

jphickey opened this issue Jun 25, 2021 · 5 comments · Fixed by #1089 or #1100
Labels
bug RTEMS unit-test Tickets related to the OSAL unit testing (functional and/or coverage)
Milestone

Comments

@jphickey
Copy link
Contributor

Describe the bug
The "reconfig" tests were added to verify that timer config calls from the context of a timer callback are rejected. Unfortunately the underlying mechanism that allows this to happen only works on POSIX (via the pthread keys, which can do this). On RTEMS and VxWorks, the mechanism which gets the task ID (OS_TaskGetId_Impl) doesn't return the timer ID when called from a timer task.

To Reproduce
Run timer tests on VxWorks or RTEMS, timer reconfig tests will fail.

Expected behavior
Test should pass.

System observed on:
MCP750 VxWorks 6.9
RTEMS 4.11.3

Additional context
Might be fixable on RTEMS but probably difficult to fix on VxWorks. May want to consider just skipping this test?

Reporter Info
Joseph Hickey, Vantage Systems, Inc.

@jphickey jphickey added the bug label Jun 25, 2021
@jphickey
Copy link
Contributor Author

For the record here is a trace of the test case:

[BEGIN] 06 TimerReconfig
[ FAIL] 06.001 ut_ostimer_timerio_test.c:87 - OS_TimerCreate(&timerId, "reconf", &g_clkAccuracy, UT_os_othertimercallback1) (-15) == OS_ERR_INCORRECT_OBJ_STATE (-35)
[ FAIL] 06.002 ut_ostimer_timerio_test.c:89 - OS_TimerAdd(&timerId, "reconf", g_timerIds[1], UT_os_othertimercallback2, NULL) (-15) == OS_ERR_INCORRECT_OBJ_STATE (-35)
[ FAIL] 06.003 ut_ostimer_timerio_test.c:90 - OS_TimerDelete(timerId) (0) == OS_ERR_INCORRECT_OBJ_STATE (-35)
[ FAIL] 06.004 ut_ostimer_timerio_test.c:91 - OS_TimerSet(timerId, 100, 100) (-16) == OS_ERR_INCORRECT_OBJ_STATE (-35)
[ FAIL] 06.005 ut_ostimer_timerio_test.c:92 - OS_TimerGetIdByName(&timerId, g_timerNames[7]) (-17) == OS_ERR_INCORRECT_OBJ_STATE (-35)
[ FAIL] 06.006 ut_ostimer_timerio_test.c:93 - OS_TimerGetInfo(timerId, &reconf->Prop) (-16) == OS_ERR_INCORRECT_OBJ_STATE (-35)
[  TTF] 06.007 ut_ostimer_timerio_test.c:278 - OS_TimerDelete(g_timerIds[2]) (-16) >= OS_SUCCESS
[  END] 06 TimerReconfig        TOTAL::7     PASS::0     FAIL::6     MIR::0     TSF::0     TTF::1     N/A::0

@skliper
Copy link
Contributor

skliper commented Jun 25, 2021

So the negative test fails in a different way, such that you still can't reconfig from within the context of a callback? Is this behavior dependable/by design? If so maybe just adjust the test to look for != OS_SUCCESS, or branch based on the first return (or the Get ID behavior) and check appropriately?

@skliper skliper added this to the 6.0.0 milestone Jun 25, 2021
@jphickey
Copy link
Contributor Author

Not quite, it is allowing the call.... which is technically not safe (it may double-take a mutex or change a linked list that is actively being traversed).

Error -15 is OS_ERR_NAME_TAKEN - this is because the attempt uses the same timer name (technically an overload, which another issue, but is easy to fix). Note that the OS_TimerDelete() actually succeeded.

My recommendation is to just better document the fact that the Timer/Timebase configuration API must not be invoked from the context of a timer. I thought it was documented, but reviewing the documentation now, I don't see it stated as clearly as it should be.

This whole check/test is only to save users from themselves and catch the programmer's error, but there's only so much that can be done in a practical sense. I would hesitate to change anything substantial at this point, for something that is a programmer error to begin with.

@skliper
Copy link
Contributor

skliper commented Jun 25, 2021

Then I'm in favor of skipping the test for RTEMS and VxWorks. I agree it's really an optional test (as-in - not verifying a "requirement") since as long as the API's are documented correctly it would be a violation of the API to attempt using these APIs in that context.

@jphickey
Copy link
Contributor Author

Unfortunately we don't have an OSAL OS type macro (e.g. _POSIX_OS), only a PSP type macro (e.g. _LINUX_OS_). Easy enough to add though, and may be useful.

jphickey added a commit to jphickey/osal that referenced this issue Jun 25, 2021
The underlying OS mechanism that rejects calls to timer APIs from
timer callbacks does not work on anything except POSIX.  This skips
these unit tests on non-POSIX platforms, and also adds documentation
clearly indicating that the API must not be called from a timer
context.
jphickey added a commit to jphickey/osal that referenced this issue Jun 25, 2021
The underlying OS mechanism that rejects calls to timer APIs from
timer callbacks does not work on anything except POSIX.  This skips
these unit tests on non-POSIX platforms, and also adds documentation
clearly indicating that the API must not be called from a timer
context.
@jphickey jphickey added the unit-test Tickets related to the OSAL unit testing (functional and/or coverage) label Jun 25, 2021
jphickey added a commit to jphickey/osal that referenced this issue Jul 7, 2021
Per review feedback, removes comment that was stale and no longer valid.
jphickey added a commit to jphickey/osal that referenced this issue Jul 7, 2021
Per review feedback, removes comment that was stale and no longer valid,
and describe why only POSIX_OS is enabled here on this check.
jphickey added a commit to jphickey/osal that referenced this issue Jul 7, 2021
Per review feedback, removes comment that was stale and no longer valid,
and describe why only POSIX_OS is enabled here on this check.
astrogeco added a commit that referenced this issue Jul 12, 2021
jphickey pushed a commit to jphickey/osal that referenced this issue Aug 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug RTEMS unit-test Tickets related to the OSAL unit testing (functional and/or coverage)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants