Fix #84, Seconds to Wakeup Count #146

tandharia · 2024-12-09T18:01:10Z

Checklist (Please check before submitting)

I reviewed the Contributing Guide.
I signed and emailed the appropriate Contributor License Agreement to GSFC-SoftwareRelease@mail.nasa.gov and copied cfs-program@lists.nasa.gov.

Describe the contribution
Fixes #84 - Updating the way the RTS commands are scheduled. Instead of using seconds to determine when a command should be executed, SC's wakeup counts are being used. Each command in an RTS is linked to a wakeup count and the command will execute only after SC has woken up a certain number of times since the previous command’s execution

Testing performed
Built and ran all tests

Expected behavior changes
RTS commands will be executed based on the number of wakeups instead of a specific time in seconds

System(s) tested on
OS: RHEL 8.10

Contributor Info - All information REQUIRED for consideration of pull request
Tvisha Andharia - GSFC 582 intern

skliper · 2024-12-09T18:04:45Z

Great work! Any requirements or design overview material need to be updated? Anything need to be updated in the BVTs? @dmknutsen aware?

config/default_sc_tbldefs.h

RichLandau · 2024-12-10T14:26:20Z

Great work! Any requirements or design overview material need to be updated? Anything need to be updated in the BVTs? @dmknutsen aware?

@tandharia please take a look through the requirements (docs\sc_FunctionalRequirements.csv) and see what needs to be updated there.

@skliper the user guide was updated as part of this. BVTs will be updated as part of the migration work currently happening.

avan989 · 2024-12-10T15:25:02Z

This approach does not account for the accumulation of error with the 1hz. (in cases where the wakeup is faster than 1 Hz)

skliper · 2024-12-10T15:45:32Z

This approach does not account for the accumulation of error with the 1hz. (in cases where the wakeup is faster than 1 Hz)

I wouldn't consider that "error". If the user calls the wakeup faster than 1Hz, that's their choice and they need to use wake-up units in their RTS. The sch_lab wakeup is very odd and does not reflect a typical use case (93 ticks or whatever it is).

RichLandau · 2024-12-10T15:51:31Z

This approach does not account for the accumulation of error with the 1hz. (in cases where the wakeup is faster than 1 Hz)

I wouldn't consider that "error". If the user calls the wakeup faster than 1Hz, that's their choice and they need to use wake-up units in their RTS. The sch_lab wakeup is very odd and does not reflect a typical use case (93 ticks or whatever it is).

@skliper do you think we should update sch_lab as part of this and make it 1 Hz to avoid any weird RTS interactions?

skliper · 2024-12-10T16:53:55Z

@skliper do you think we should update sch_lab as part of this and make it 1 Hz to avoid any weird RTS interactions?

@RichLandau I think updating sch_lab to use more common wake-up timing would be good. There's a set of weird ones that look to be for testing sch_lab only. I don't think it has to be in sync w/ this change, but a good update either way.

dzbaker · 2024-12-10T17:17:59Z

Great work! Any requirements or design overview material need to be updated? Anything need to be updated in the BVTs? @dmknutsen aware?

@tandharia please take a look through the requirements (docs\sc_FunctionalRequirements.csv) and see what needs to be updated there.

@skliper the user guide was updated as part of this. BVTs will be updated as part of the migration work currently happening.

I don't immediately see any requirements in the csv file that will need changing (only a few that look like there could have been discrepancies for due to SC#84), but it would be good for @tandharia to double-check that this modification didn't change anything in the requirements.

tandharia · 2024-12-10T17:56:49Z

Great work! Any requirements or design overview material need to be updated? Anything need to be updated in the BVTs? @dmknutsen aware?

@tandharia please take a look through the requirements (docs\sc_FunctionalRequirements.csv) and see what needs to be updated there.
@skliper the user guide was updated as part of this. BVTs will be updated as part of the migration work currently happening.

I don't immediately see any requirements in the csv file that will need changing (only a few that look like there could have been discrepancies for due to SC#84), but it would be good for @tandharia to double-check that this modification didn't change anything in the requirements.

I looked over the csv file and thought some lines needed some updating because of this pull request and have committed those updates

RichLandau · 2024-12-10T18:03:56Z

docs/sc_FunctionalRequirements.csv

-SC2002.2,SC2002.2,SC shall accept variable length packed RTS commands within the <PLATFORM_DEFINED> byte relative time-tagged sequences.,
+d)       Command number",SC application needs to validate the table to ensure gross errors are caught.  cFE Table Services handles the ground interface to tables. 
+SC2002,SC2002,SC shall allocate <PLATFORM_DEFINED> Relative Time-tagged Sequences (RTSs) with each capable of storing <PLATFORM_DEFINED>  bytes of stored command data.,Specifies the limits for table sizing purposes.
+SC2002.1,SC2002.1,SC shall resolve time to a wakeup count for Relative Time Command Sequences (RTS).,To address the bug where RTS commands could be delayed when the 1 Hz wakeup occurs near a second rollover.


Typically in requirements we wouldn't specify the "why," just the "what," In this case it isn't resolving time at all anymore. I would word this to say

SC shall track a wakeup count for Relative Time Command Sequences (RTS)

RichLandau · 2024-12-10T18:04:57Z

docs/sc_FunctionalRequirements.csv

-SC2005.1,SC2005.1,"SC shall defer execution of pending RTS commands, when the combined execution count, of ATS and RTS, exceeds the command per second limit.",
-SC2005.2,SC2005.2,SC shall allow up to the maximum number of defined RTSs to be active concurrently.,RTSs can run concurrently. The limitation is the number of commands that can be sent in one second.
+a)       A relative wakeup count
+b)       A variable length command, with a maximum length of <PLATFORM_DEFINED> bytes",Each command within an RTS needs to contain a wakeup count.


You can drop the text after the comma for b), this restriction is lifted by moving to wakeup counts. a) already covered needing one.

RichLandau · 2024-12-10T18:05:52Z

docs/sc_FunctionalRequirements.csv

-SC4001.2,SC4001.2,"For the first command in an RTS table, the delay time shall be relative to the receipt of the RTS Start Command.",Provides the ability to delay the executing of the first command.
+Having a config parameter allows a range of RTSs to be specified such that no event message is sent."
+SC4000.2,SC4000.2,"If the conditions are not met, SC shall reject the command and send an event message.",Ground needs to know if an RTS was not executed.
+SC4001,SC4001,"SC shall dispatch commands within the RTS table, in position order, as the relative wakeup count specified in the RTS command expires.",RTS contain relative wakeup counts between commands.


I would say "wakeup count... elapses" instead of expires, counts don't really expire.

RichLandau · 2024-12-10T18:07:46Z

docs/sc_FunctionalRequirements.csv

@@ -135,15 +135,15 @@ p)       RTS table activation count
 q)       RTS table activation error count
 r)        Number of active RTSs
 s)       Identifier of the next RTS table to dispatch a command
-t)        Time the next RTS command is due to be dispatched
+t)        Wakeup count the next RTS command is due to be dispatched


Wording is a little weird here, maybe "Wakeup count delay after which the next RTS command..."?

RichLandau · 2024-12-10T18:09:33Z

docs/sc_FunctionalRequirements.csv

+SC4000.2,SC4000.2,"If the conditions are not met, SC shall reject the command and send an event message.",Ground needs to know if an RTS was not executed.
+SC4001,SC4001,"SC shall dispatch commands within the RTS table, in position order, as the relative wakeup count specified in the RTS command expires.",RTS contain relative wakeup counts between commands.
+SC4001.1,SC4001.1,The wakeup count shall be interpreted as the number of wakeup events to delay relative to the previous RTS command dispatched from that RTS table.,
+SC4001.2,SC4001.2,"For the first command in an RTS table, the delay shall be relative to the receipt of the RTS Start Command.",Provides the ability to delay the executing of the first command.
 SC4001.3,SC4001.3,"Prior to the dispatch of each individual RTS command, SC shall verify the validity of the following command parameters:


Disregard, see thread below

I would add a new requirement that says "If an ATS and RTS both meet the criteria to execute, the ATS will be executed and the RTS will be delayed until the next wakeup."

That locks in the behavior we have now so it's not a surprise, and if we ever want to change that behavior we know the baseline.

@RichLandau is that true? Do you mean the command to start the ATS/RTS, or if commands within the ATS/RTS would go out the same wakeup? I think in SC_OneHzWakeupCmd (at least the version before this change) it'll process up to SC_MAX_CMDS_PER_SEC from either ATS or RTS.

Good point, I was looking at SC_UpdateNextTime in the diff without seeing the loop it was in. Looking here:
https://github.com/nasa/SC/pull/146/files#diff-dacb535e36ef925e9071b7ca9d813f065b6eeeb756dbd506a2dc3743b004cd03R135

It looks like ATPs are processed first, and then (previously) RTPs were scheduled if they came before the ATP. The logic now schedules the RTP if there isn't an ATP scheduled. Do you think this will function the same as before?

@RichLandau - I don't think so, although the logic confuses me a bit and seems unnecessarily complex. Why not in the wakeup just do the switch if pending, send the Atp cmds in a loop until done or SC_MAX_CMDS reached, then send Rtp commands in a loop until done or SC_MAX_CMDS reached?

I guess the Switch needs to be inside the Atp cmd loop since you can switch from an Atp... but otherwise, why not split into 2 loops?

Apologies for the spamming... but maybe it does need to be in one loop if you can command an ATS switch from an RTS. Even if that's the case the loop could still be simplified to switch, then send an absolute command if the next cmd time is <= current time, and if not send an rts command if next wakeup is <= current wakeup. Exit once either no command is sent or SC_MAX_CMDS is met.

I would expect them to be in order, if there is an RTS with 1 tick delay followed by an ATS with a timeout that has expired, I'd expect them to fire RTS and then ATS.

I don't completely understand your example, but I think what you are describing would be changing current behavior. Right now if ATS and RTS are both due in a wakeup, it does ATS first then cycles through all the RTS's in order until all due commands are sent or it hits the command limit. ATS command is higher priority than RTS 5 is higher priority than RTS 20. There isn't a calculation at a lower level of if the ATS absolute time is actually before or after the RTS... just that they are due on the same wakeup.

config/default_sc_internal_cfg.h

fsw/src/sc_cmds.c

fsw/tables/sc_rts002.c

unit-test/stubs/sc_app_stubs.c

unit-test/sc_atsrq_tests.c

fsw/src/sc_utils.c

@@ -100,7 +100,7 @@
 /* Compute Absolute time from relative time                       */
 /*                                                                 */
 /* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * */
-SC_AbsTimeTag_t SC_ComputeAbsTime(SC_RelTimeTag_t RelTime)
+SC_AbsTimeTag_t SC_ComputeAbsTime(uint32 RelTime)


fsw/src/sc_cmds.c

 /*                                                                 */
 /* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * */
-void SC_OneHzWakeupCmd(const SC_OneHzWakeupCmd_t *Cmd)
+void SC_WakeupCmd(const SC_WakeupCmd_t *Cmd)


fsw/src/sc_cmds.c

+    while (SC_OperData.NumCmdsSec < SC_MAX_CMDS_PER_WAKEUP)
    {
+        SC_UpdateNextTime();
+
+        CurrentNumCmds = SC_OperData.NumCmdsSec;
+
        /*
         *  Check to see if there is an ATS switch Pending, if so service it.
         */
        if (SC_OperData.AtsCtrlBlckAddr->SwitchPendFlag == true)
        {
            SC_ServiceSwitchPend();
        }

-        if (SC_AppData.NextProcNumber == SC_Process_ATP)
-        {
-            SC_ProcessAtpCmd();
-        }
-        else
-        {
-            if (SC_AppData.NextProcNumber == SC_Process_RTP)
-            {
-                SC_ProcessRtpCommand();
-            }
-        }
+        SC_ProcessAtpCmd();

-        SC_UpdateNextTime();
-        if ((SC_AppData.NextProcNumber == SC_Process_NONE) ||
-            (SC_AppData.NextCmdTime[SC_AppData.NextProcNumber] > SC_AppData.CurrentTime))
-        {
-            SC_OperData.NumCmdsSec         = 0;
-            IsThereAnotherCommandToExecute = false;
-        }
-        else /* Command needs to run immediately */
-        {
-            if (SC_OperData.NumCmdsSec >= SC_MAX_CMDS_PER_SEC)
-            {
-                SC_OperData.NumCmdsSec         = 0;
-                IsThereAnotherCommandToExecute = false;
-            }
-            else
-            {
-                IsThereAnotherCommandToExecute = true;
-            }
+        SC_ProcessRtpCommand();
+
+        /* 
+         * No commands could be processed
+         */
+        if (CurrentNumCmds == SC_OperData.NumCmdsSec) {
+            break;
        }
-    } while (IsThereAnotherCommandToExecute);
+    }


fsw/src/sc_cmds.c

skliper · 2024-12-13T14:03:58Z

Great work! I don't have any other comments. Once functionally verified it's good to go from my perspective!

RichLandau reviewed Dec 10, 2024

View reviewed changes

config/default_sc_tbldefs.h Outdated Show resolved Hide resolved

Fix nasa#84, Seconds to Wakeup Count

be76457

tandharia force-pushed the fix-84-seconds-to-wakeups branch from 5822f43 to be76457 Compare December 10, 2024 17:42

RichLandau reviewed Dec 10, 2024

View reviewed changes

Fix nasa#84, Formatting

2212070