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

Clarify/document various IDs and Indices and Numbers used internally #116

Closed
3 tasks done
jphickey opened this issue Oct 10, 2023 · 0 comments · Fixed by #127
Closed
3 tasks done

Clarify/document various IDs and Indices and Numbers used internally #116

jphickey opened this issue Oct 10, 2023 · 0 comments · Fixed by #127
Assignees

Comments

@jphickey
Copy link
Contributor

Checklist (Please check before submitting)

  • I reviewed the Contributing Guide.
  • I reviewed the README file to see if the feature is in the major future work.
  • I performed a cursory search to see if the feature request is relevant, not redundant, nor in conflict with other tickets.

Is your feature request related to a problem? Please describe.
The SC source code uses lots of different values to identify RTSs, ATSs, and Commands within them. Some are 0-based (usually for indexing into internal arrays) and some are 1-based (usually for telemetry).

But usage isn't entirely consistent. The type used to store these varies - sometimes uint8, but sometimes uint16 or even a signed value, with conversions/assignments between them, scattered all over the source.

There are conversion macros (e.g. SC_ATS_ID_TO_INDEX) but these do not seem to be applied totally consistently. Furthermore, there is some overlap between these - SC_RTS_INDEX_TO_NUM and SC_RTS_INDEX_TO_ID appear to do exactly the same thing. NUM and ID both appear to refer to a 1-based identifier and there is no documented distinction here why these two exist.

Describe the solution you'd like
First and foremost - Document what is meant by an "ID" and an "INDEX" and a "NUM" and where each one should be used. Scrub through the code and make sure that conversion macros are consistently used, and the right variant is used in the right context. Make sure a consistent data type is used to store the identifier.

Furthermore - to improve code robustness, at least Internally within SC - use a unique (type-safe) data type to refer to each identifier. This will restrict implicit conversions or comparisons between them, allowing any mismatches to be caught at compile time. This ensures that any conversion is intentional, and must use the correct conversion macro to perform the conversion.

Requester Info
Joseph Hickey, Vantage Systems, Inc.

@jphickey jphickey self-assigned this Oct 10, 2023
jphickey added a commit to jphickey/SC that referenced this issue Nov 9, 2023
Introduces header files and dedicated types for the purpose of indexing
and identification of SC objects and structures.  This include ATS/RTS
identification, time sequence numbers, word offsets, and command
numbers.

Use a separate typedef for each, and document its purpose/intent in the
description, then update all FSW to use the typedef rather than a simple
uint16.

Note - in places where the CMD/TLM interface were _not_ using a uint16,
the old type is kept.  This maintains backward compatibility of the I/F.
jphickey added a commit to jphickey/SC that referenced this issue Nov 9, 2023
Introduces header files and dedicated types for the purpose of indexing
and identification of SC objects and structures.  This include ATS/RTS
identification, time sequence numbers, word offsets, and command
numbers.

Use a separate typedef for each, and document its purpose/intent in the
description, then update all FSW to use the typedef rather than a simple
uint16.

Note - in places where the CMD/TLM interface were _not_ using a uint16,
the old type is kept.  This maintains backward compatibility of the I/F.
dzbaker added a commit that referenced this issue Dec 5, 2023
Fix #116, 117+118 - Multiple cleanups related to array indexing and range checking
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant