Add Svc.Version component #2747
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| break; | ||
| } | ||
|
|
||
| this->cmdResponse_out(opCode, cmdSeq, Fw::CmdResponse::OK); |
Check warning
Code scanning / CodeQL
Unchecked function argument Warning
| break; | ||
| } | ||
|
|
||
| this->cmdResponse_out(opCode, cmdSeq, Fw::CmdResponse::OK); |
Check warning
Code scanning / CodeQL
Unchecked function argument Warning
…rule isnt violated
|
@LeStarch and @thomas-bc : I am not sure why I am seeing CI/Framework fail on Svc_Version_ut_exe. I've run it a few times and passes just fine on my local branch |
|
|
||
| Version ::~Version() {} | ||
|
|
||
| void Version::config(bool enable) { |
Check notice
Code scanning / CodeQL
Use of basic integral type Note
| // ---------------------------------------------------------------------- | ||
|
|
||
| void Version ::ENABLE_cmdHandler(FwOpcodeType opCode, U32 cmdSeq, Svc::VersionEnabled enable) { | ||
| this->m_enable = (enable == VersionEnabled::ENABLED); |
Check warning
Code scanning / CodeQL
Unchecked function argument Warning
|
|
||
| void Version::config(bool enable) { | ||
| // Set Verbosity for custom versions | ||
| this->m_enable = enable; |
Check warning
Code scanning / CodeQL
Unchecked function argument Warning
| } | ||
| } | ||
|
|
||
| Version ::~Version() {} |
Check notice
Code scanning / CodeQL
More than one statement per line Note
| this->tlmWrite_ProjectVersion(proj_tlm); | ||
| } | ||
|
|
||
| void Version ::libraryVersion_tlm() { |
Check notice
Code scanning / CodeQL
Long function without assertion Note
Deleting unneeded test helper file
| this->cmdResponse_out(opCode, cmdSeq, Fw::CmdResponse::OK); | ||
| } | ||
| // ---------------------------------------------------------------------- | ||
| // implementations for internal functions |
There was a problem hiding this comment.
Nit: internal functions being named *_tlm() is a little confusing imo since they do both telemetry and event. Maybe _downlink() or sendAll() or something similar?
There was a problem hiding this comment.
Sorry, I am so used to using telemetry as a generic term to describe data for users, which would mean both EVRs and EHAs.
There was a problem hiding this comment.
Actually you're right - I've kind of overloaded the term telemetry in my head to mean telemetry channel...
Confusion is probably coming from the tlmWrite_*() name of the function to write to a channel. But our EVR/event scheme would indeed fall under the Telemetry umbrella in, say, the CCSDS standards.
Anyways, I'm ok with anything!
| struct CustomVersionDb { | ||
| version_enum: VersionCfg.VersionEnum @<enumeration/name of the custom version | ||
| version_value: string size 80 @< string containing custom version | ||
| version_status : Svc.VersionStatus @< status of the custom version |
There was a problem hiding this comment.
What is the purpose of version_status in this struct?
This seems to be used-specified when setting a custom version so I don't quite understand the use case - is there a scenario where a user would set a value for a version but also set a FAILURE status?
There was a problem hiding this comment.
If it carries meaning, should it be downlinked along with the version_value in customVersion_tlm() ?
There was a problem hiding this comment.
Yeah I debated about having this, in the end decided to leave it so it could be project specific. I figured this would carry more meaning for when "getVersion" is called rather than providing a downlink for the status
| @@ -46,6 +46,7 @@ add_fprime_subdirectory("${CMAKE_CURRENT_LIST_DIR}/StaticMemory/") | |||
| add_fprime_subdirectory("${CMAKE_CURRENT_LIST_DIR}/TlmChan/") | |||
| add_fprime_subdirectory("${CMAKE_CURRENT_LIST_DIR}/TlmPacketizer/") | |||
| add_fprime_subdirectory("${CMAKE_CURRENT_LIST_DIR}/SystemResources/") | |||
| add_fprime_subdirectory("${CMAKE_CURRENT_LIST_DIR}/Ports/VersionPorts") | |||
There was a problem hiding this comment.
Could be good to add a CMakeLists.txt under Svc/Ports/ so that we can only include
add_fprime_subdirectory("${CMAKE_CURRENT_LIST_DIR}/Ports")
here, and future ports can easily be added under the Svc/Ports module
There was a problem hiding this comment.
Is this pointed to VersionPorts or are you saying in general?
| } | ||
|
|
||
| void Version ::customVersion_tlm_all() { | ||
| for (U8 i = 0; (m_enable == true) && (m_num_custom_elements != 0) && (i < Svc::VersionCfg::VersionEnum::NUM_CONSTANTS); |
There was a problem hiding this comment.
Reading some parts of the docs/code I was under the impression that m_enable would control whether to send both telemtry and events, or only events.
Currently it looks like nothing will be downlinked unless m_enable is true.
Should there be a condition that still does a minimal amount of downlinking (maybe send an event only but no telemetry?) if the user sends a command VERSION(CUSTOM/ALL) when m_enable is false?
For example I can see that users would expect to get some amount of info back if they specifically send VERSION(CUSTOM), regardless of the verbose status.
There was a problem hiding this comment.
That's a great point. One way the users can get custom version is to first send the enable command and then ask for VERSION(CUSTOM) but that seems like a cumbersome way of doing it. I think when a user sends a command to request custom version, it should be sent directly. @LeStarch: Thoughts?
The only way the telemetry is queued for downlink is during startup (all tlm downlinked except custom), port call and ground command. So essentially m_enable would only prevent verbosity during port calls.
| // function to log custom version events and channels | ||
| void customVersion_tlm(VersionSlot custom_slot); | ||
| void customVersion_tlm_all(); | ||
| bool m_enable; /*!<Send TLM when true>*/ |
Check notice
Code scanning / CodeQL
Use of basic integral type Note
| // Component construction and destruction | ||
| // ---------------------------------------------------------------------- | ||
|
|
||
| Version ::Version(const char* const compName) |
Check notice
Code scanning / CodeQL
Use of basic integral type Note
Change Description
Implementation for ticket : #2604
Rationale
Migrating Versions into its own component
Testing/Review Recommendations
Ensure the functionality for the new versions component is as discussed.
Future Work
Once Versions component is approved, the remnants in SystemResources will be removed.