Fix #648, 649, 664 - refactor all table array access #666
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jphickey
force-pushed
the
fix-648-649-664
branch
2 times, most recently
from
40902ed to
cf9db6d
Compare
jphickey
added
the
CCB:Ready
osal Integration Candidate: 2020-11-24
Introduce the OS_object_token_t type which tracks a reference to an OSAL resource. This contains all information about the original reference, including the ID, object type, lock type, and the table index. Therefore, since the token type contains all relevant info, it can be used in all places where a bare index was used. This also considerably simplifies the code, as some functions which previously output multiple objects only need to operate on tokens, and the functions which called these functions only need to instantiate a token.
Use a variable name that matches the type of resource being accessed, rather than just "local". In particular this is important for readability of timecb and timebase code where functions need often need to access both types of objects at the same time. This also updates filesys code to match.
jphickey
force-pushed
the
fix-648-649-664
branch
from
cf9db6d to
1f6bf80
Compare
|
Rebased to latest |
astrogeco
added
CCB-20201202
and removed
CCB:Ready
|
CCB 2020-12-02 APPROVED
|
skliper
approved these changes
Dec 3, 2020
|
This also fixes #647 although that wasn't in a separate commit -- By using an iterator this is now invoking |
Pass token objects to impl layers to identify the object to operate on, rather than the index. The token has extra information including the original/actual object ID - which matters if the ID might change as part of the operation (e.g. new/delete).
For the linked list of timer callbacks, use the full object ID value rather than just the index. This ensures consistency in the list and also makes it easier to manage.
ID is preferable to an array index because direct table access should not be done outside OSAL itself.
jphickey
force-pushed
the
fix-648-649-664
branch
from
1f6bf80 to
ba3bc01
Compare
|
Found a minor issue here where it was filling the active_id field with RESERVED value ... corrected in commit ffb098d (actually didn't cause any breakage here but it did break my next PR for next week). |
|
@acudmore can you review this? |
astrogeco
added a commit
to nasa/cFS
that referenced
this pull request
Dec 9, 2020
astrogeco
added a commit
to nasa/cFS
that referenced
this pull request
Dec 9, 2020
jphickey
added a commit
to jphickey/osal
that referenced
this pull request
Aug 10, 2022
The "CFE_SB_CmdHdr_t" and "CFE_SB_TlmHdr_t" types were not defined such that they would have compatible alignment with (and thereby allow safe casting to/from) a CFE_SB_Msg_t type. This changes the definition to be a union so that the types are aligned correctly.
jphickey
added a commit
to jphickey/osal
that referenced
this pull request
Aug 10, 2022
Update the CFE_ES_ShellTlm_t, CFE_TIME_ToneSignalCmd_t, and CFE_TIME_FakeToneCmd_t to use the CFE_SB_TlmHdr_t/CFE_SB_CmdHdr_t types to define the buffer, rather than a uint8 array. This should not change the size, as it was already defined using sizeof() this structure, but it will make it aligned correctly which resolves the compiler warning. Note that all CMD/TLM should really be defined this way, but this only selectively changes the places that were actually generating a compiler warning about this. There is a risk that padding will be added, but this change should not change the padding or size of messages in 32-bit builds.
jphickey
added a commit
to jphickey/osal
that referenced
this pull request
Aug 10, 2022
Update the CFE_ES_ShellTlm_t, CFE_TIME_ToneSignalCmd_t, and CFE_TIME_FakeToneCmd_t to use the CFE_SB_TlmHdr_t/CFE_SB_CmdHdr_t types to define the buffer, rather than a uint8 array. This should not change the size, as it was already defined using sizeof() this structure, but it will make it aligned correctly which resolves the compiler warning. Note that all CMD/TLM should really be defined this way, but this only selectively changes the places that were actually generating a compiler warning about this. There is a risk that padding will be added, but this change should not change the padding or size of messages in 32-bit builds.
jphickey
pushed a commit
to jphickey/osal
that referenced
this pull request
Aug 10, 2022
Fix nasa#666, alignment of CMD/TLM message definitions
jphickey
pushed a commit
to jphickey/osal
that referenced
this pull request
Aug 10, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Describe the contribution
Refactor the table array access across OSAL. Multiple fixes combined into one PR as they depend on each other.
Fix #648 : Rather than indexing arrays directly, use a token concept in combination with a macro to obtain the table entry. All access is then done through this table pointer. The token contains all relevant information about an object, importantly it has the "truth" as to what actual object ID is being manipulated. This is necessary when operations such as new/delete may change the value in the table itself - the token always has the right value.
Fix #649 : Use the full object ID in the timer call back list. This avoids complexities (and issues) associated with trying to regenerate the ID when the list changes.
Fix #664 : Update the timer sync callback prototype. Pass the entire OSAL ID to the sync function, not just the index. This is technically an API change (although they are both uint32, the value changes). AFAIK this sync feature isn't actively used so this shouldn't really have an impact though.
Testing performed
Build and run all tests for native (POSIX), rtems 4.11 and VxWorks
Expected behavior changes
No impact to behavior - just a substantial cleanup of the internal patterns.
System(s) tested on
Ubuntu 20.04, rtems 4.11
Additional context
This is also necessary as a prerequisite to a couple other OSAL issues - where the original ID needs to be preserved across ops which create/delete entities. The token provides this.
Contributor Info - All information REQUIRED for consideration of pull request
Joseph Hickey, Vantage Systems, Inc.