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

Speed up boot time #942

Closed
wants to merge 5 commits into from

Conversation

Yosty7B3
Copy link
Collaborator

@Yosty7B3 Yosty7B3 commented Mar 27, 2023

Similar as #732: oc_states changes to speed up boot time.

Difference this time:

  • keeping the settings editor in oc_states
  • no longer requiring ALIVE from oc_settings

The boot sequence goes through 3 stages.

  • First state a REBOOT signal is send and waits for ALIVE from oc_core and oc_api. And also check if oc_settings is running
  • In the 2e state the STARTUP signal is sent and waits for replies
  • Entering the final state oc_state sends out the TIMEOUT_READY signal.

Similar as OpenCollarTeam#732: oc_states changes to speed up boot time. 

Difference this time:
* keeping the settings editor in oc_states
* no longer requiring ALIVE from oc_settings

Additionally, the settings editor reads and saves the settings directly from LinksetData (since it's been added in setting OpenCollarTeam#941)

The boot sequence goes through 3 stages.
* First state a REBOOT signal is send and waits for ALIVE from oc_core and oc_api. And also check if oc_settings is running
* In the 2e state the STARTUP signal is sent and waits for replies
* Entering the final state oc_state sends out the TIMEOUT_READY signal.
The button wasn't getting added because the MENUNAME_REQUEST was caught to late in the script. This should fix that.
@Medea-Destiny Medea-Destiny added Do Not Merge (yet) One reason or another for not merging OC9.x Target v9 with LSD revisions labels Apr 8, 2023
@Medea-Destiny
Copy link
Collaborator

Nice work. Pending final decisions on settings namespace. LSD will get used for things we don't want to expose to the settings editor, so this will take some thought and planning - the tokens accepted into the editor should be limited by prefix.

I'm rather hoping that the LSD changes are going to make it easier for us to skip the boot process altogether, but we'll see.

@Yosty7B3 Yosty7B3 changed the title Speed up boot time & Linkset Data integration Speed up boot time Jul 26, 2023
Removing LinksetData dependency so it hopefully can be used in 8.3
@Yosty7B3
Copy link
Collaborator Author

Yosty7B3 commented Jul 26, 2023

Nice work. Pending final decisions on settings namespace. LSD will get used for things we don't want to expose to the settings editor, so this will take some thought and planning - the tokens accepted into the editor should be limited by prefix.

I'm rather hoping that the LSD changes are going to make it easier for us to skip the boot process altogether, but we'll see.

Seeing how 9.x is probably far of, I figured I remove the LSD dependencies for now.
The editor is restored as it was in 8.2.3, while still keeping the speed up changes.

Please check if it can be merged with 8.3 now.

@Medea-Destiny
Copy link
Collaborator

@Yosty7B3 I've taken a good look at this now (sorry it has taken so long to test it thoroughly!) I like the way you've approached this and with this change a collar will start reacting properly in about 6-7 seconds while 8.2.3 takes 14-15 seconds.
However, the "Collar Ready. Startup Complete" message is taking on average about an extra 20 seconds compared to 8.2.3 to appear (4 test runs, 51, 53, 53 & 47 seconds, compared to 30, 30, 30 & 29 with 8.2.3). I'd have to dig deeper than I have so far to be 100% sure why, but it looks like without the ALIVE message timing paddings, the initialize message is being sent repeatedly, restarting the sending of settings from the settings script. I haven't checked but I'd imagine this is generating a ton of extra link_message spam to at boot time, and the later arrival of the startup complete message is likely to generate complaints even if the collar is realistically usable more quickly. The optics are likely to be a problem.

If my guess is right about the cause here, this should be fixable by putting in a delay after receiving an ALIVE signal before sending an INITIALIZE signal rather than sending one per ALIVE. I think it should be practical via your methods to get the collar to a stable boot up in around 20 seconds, which people will appreciate. However as we will be simplifying the boot process significantly for 9.x there's a question of how much effort it would take you to refine this if it turns out to be more intractable than my guess, though I'd hate the work you've done to be wasted.

One final side point: when doing PRs, can we please focus on one thing at a time. This particularly in the case of reformatting. Your version of the script is much more user-friendly keeping the global variables all at the top rather than mixed in with the functions, but it makes it a lot harder to do a code review. That kind of reformatting should be done as a separate PR first without any code changes at the same time, because otherwise the diffs are kind of a nightmare.

Thanks for your excellent work on this so far.

@Yosty7B3
Copy link
Collaborator Author

Yosty7B3 commented Nov 9, 2023

Hmmm yes. For some reason this didn't happen to me but using a fresh collar and only changing oc_states I saw it too now.
You were right it was the initialize signal.
In the running state, the script is responding to every ALIVE message it receives by sending an initialize.

        } else if(iNum == ALIVE){
            llMessageLinked(LINK_SET, STARTUP, sStr, "");
            llSleep(1);
            llMessageLinked(LINK_SET, 0, "initialize", llGetKey());

I can probably remove that whole part. I don't remember exactly what my thinking was but I assume it was intended as a failsafe in case a single script got restarted.

@Medea-Destiny
Copy link
Collaborator

@Yosty7B3 The initialize message ensures that scripts assert menus and request settings as necessary. I think in the original oc_states the idea was that if a script starts late and sends its ALIVE message after the initialize message has been sent, oc_states will resend initialize to ensure everything's synced up. Ideally the initialize message would be sent once all scripts have reported ALIVE, but that would suspend the boot process if any script exists that for whatever reason doesn't send an ALIVE message -- a 3rd party texture script or something. If the initialize is not sent immediately an ALIVE message is received but rather that sets a flag to fire initialize on the next timer tick as long as say 5 seconds has passed since the last ALIVE message was received (and probably shorten the timer tick) that should resolve it.

@Yosty7B3
Copy link
Collaborator Author

Yea, initialize does trigger the menu update. So I guess it should stay in then.
I'll edit it to add an timer check.

"initialize" was being spammed on every script that sent an ALIVE signal, and "STARTUP ALL" was being send before oc_api was even alive.
Changed it so that STARTUP and initialize are only send once.
@SilkieSabra SilkieSabra deleted the branch OpenCollarTeam:8.3_Features-branch October 5, 2024 16:03
@SilkieSabra SilkieSabra closed this Oct 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Do Not Merge (yet) One reason or another for not merging OC9.x Target v9 with LSD revisions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants