Skip to content

Support keep alive messages from the desktop app#274

Merged
DocMoebiuz merged 8 commits intoMobiFlight:mainfrom
neilenns:neilenns/issue273
Nov 18, 2023
Merged

Support keep alive messages from the desktop app#274
DocMoebiuz merged 8 commits intoMobiFlight:mainfrom
neilenns:neilenns/issue273

Conversation

@neilenns
Copy link
Contributor

@neilenns neilenns commented Nov 10, 2023

Fixes #273

This PR fundamentally changes how the firmware knows when to keep displays awake.

Instead of relying on the firmware to track when various events happen (and missing some events, like buttons, encoders, and multiplexers), it simply relies on the desktop app to tell it to stay awake.

The companion desktop app change sends a keep alive message every 5 minutes. If it's been 15 minutes since one of those messages came in the firmware will put displays to sleep.

Bonus: Removing all the calls to setLastCommandMillis saves about 1k of space (at least for the pico)

@github-actions
Copy link

Firmware for this pull request:
Firmware.zip

@neilenns neilenns marked this pull request as ready for review November 10, 2023 22:47
@neilenns neilenns requested a review from DocMoebiuz as a code owner November 10, 2023 22:47
@github-actions
Copy link

Firmware for this pull request:
Firmware.zip

@neilenns neilenns requested a review from elral November 10, 2023 22:48
@elral
Copy link
Collaborator

elral commented Nov 11, 2023

I still vote that the connector actively sets the power mode, e.g. by:
void OnSetPowerSavingMode() { SetPowerSavingMode(cmdMessenger.readBoolArg());}

The connector knows when to enter the power safe mode in 15min. by not sending keep alive messages anymore. So the connector can also the time when to send the message to enter the power sage mode. This time is for now 15min which is comiled into the firmware, now it could be an user choosable time from the settings dialog (or just still the fixed 15min.).
To leave the power saving mode the connector sends the required message instead of starting keep alive messages.
This would also reduce the serial traffic.

If it would be done according this, there would be the follwoing changes:

  • CommandMessenger.cpp
    ** The function like above
    ** getLastCommandMillis() and void setLastCommandMillis()would not be needed anymore
  • Mobiflight.cpp
    ** void updatePowerSaving() would not be needed anymore
    ** updatePowerSaving(); in the main loopwould not be needed anymore
    ** SetPowerSavingMode(false); to be added in ResetBoard()
  • Mobiflight.h
    ** void SetPowerSavingMode(bool state);to be added

/edit:
For backward compatibility the connector has still to send regulary a message for setting the power mode on, and to stop this message 15min. before the message is sent to enter power saving mode for boards with updated formware.

Hmhm, and the connector has to now if it is the new firmware and if the command to enter the power saving mode can be send, otherwise older firmwares will leave again the power saving mode due to an unknown command. Not really nice...

@neilenns
Copy link
Contributor Author

I thought about changing to have the desktop manage everything, including putting things to sleep, but decided against it for a couple of reasons:

  1. Backwards compatibility is challenging (as you mentioned in your edit)
  2. If the connector ever crashes for some reason (which sadly does happen) the boards will never get a go to sleep message and will stay on permanently

There is very little serial traffic with the current approach, it's just one message every 5 minutes. We could even safely extend it to 10 or even 14 minutes if we wanted to I think.

@DocMoebiuz
Copy link
Collaborator

I originally implemented the power saving mode to make sure that if anything stays on, it will turned off.

This has to work without Mobiflight Connector running.

The Connector should prevent Power Saving Mode while in Run Mode.

If the connector crashes and if it could not send a proper stop command - the boards should turn all outputs off - they have to be able to do it without the connector.

I don't see a reason why the Connector should ever send a PowerSavingMode ON message. The Connector would be in STOPped mode for 15 minutes and then send a PowerSavingMode ON. But it is in STOP - so no timers are really running etc

@elral
Copy link
Collaborator

elral commented Nov 12, 2023

  1. If the connector ever crashes for some reason (which sadly does happen) the boards will never get a go to sleep message and will stay on permanently

I thought also about this. I think after a crashed connector the most user will restart the connector. Yeah, but not always. So indeed better to have the keep alive message.

There is very little serial traffic with the current approach, it's just one message every 5 minutes. We could even safely extend it to 10 or even 14 minutes if we wanted to I think.

That's true, compared to other devices it's nearly nothing.

What we can think about to reduce the time in the FW to 1 minute and sending every e.g. 30sec. Then the user could define the time when the boards should go sleep in the settings, with a minum of one minute and a tolerance of 30 sec.
But maybe again over engineered ;)

@DocMoebiuz
Copy link
Collaborator

Why can't we just make it work as it was intended? That would be the best. This feature was never meant to be really "visible" to the user.

Being able to set the timeout period - what's the use case?

At the moment it's not working and people are noticing the feature because of its bug. Once we fix the bug and it behaves as intended no one will care anymore about it.

In my opinion there is no need to make this configurable.

Send a keep alive message every 5 mins. Reset the timeout in the firmware. Done.

What am I missing?

@elral
Copy link
Collaborator

elral commented Nov 12, 2023

I am also fine with this approach

@neilenns
Copy link
Contributor Author

Send a keep alive message every 5 mins. Reset the timeout in the firmware. Done.

That's what I did 😀

Copy link
Collaborator

@DocMoebiuz DocMoebiuz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comments please.

@github-actions
Copy link

Firmware for this pull request:
Firmware.zip

@github-actions
Copy link

Firmware for this pull request:
Firmware.zip

@neilenns neilenns requested a review from DocMoebiuz November 12, 2023 23:08
@DocMoebiuz DocMoebiuz merged commit 4d1a944 into MobiFlight:main Nov 18, 2023
@DocMoebiuz DocMoebiuz added the enhancement New feature or request label Nov 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Buttons, encoders, and diginmux actions do not prevent power save mode

3 participants