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

Async Refactor WIP #75

Merged
merged 58 commits into from
Oct 14, 2023
Merged

Conversation

fel115
Copy link
Collaborator

@fel115 fel115 commented Aug 28, 2023

No description provided.

horenso and others added 3 commits August 28, 2023 16:57
This keeps the old semantic with normal/senor mode
but makes main more understandable. It's a good first
step towards a refactor.
@horenso
Copy link
Collaborator

horenso commented Aug 29, 2023

Seems like it only needs formatting.

@horenso
Copy link
Collaborator

horenso commented Aug 29, 2023

We can merge this to have one consistent "old" version before the complete refactor.

@markus2330
Copy link
Contributor

markus2330 commented Aug 30, 2023

We can merge this to have one consistent "old" version before the complete refactor.

If it is tested on hardware that it still works, we can do so, otherwise I don't see why we should do it. Master should be known to work. You can simply give v0.8.* tags on your branch when something is consistent. When we finish this refactoring, we can open v0.9.* for testing/fixing (then again on master) and finally we can release 1.0.0.

Copy link
Contributor

@markus2330 markus2330 left a comment

Choose a reason for hiding this comment

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

Good to see progress, cannot wait to see more 🚀

files/opensesame.spec Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/mod_ir_temp.rs Outdated Show resolved Hide resolved
src/todo Outdated Show resolved Hide resolved
src/todo Outdated Show resolved Hide resolved
@fel115 fel115 changed the title Refactor main Async Refactor WIP Aug 31, 2023
@horenso
Copy link
Collaborator

horenso commented Aug 31, 2023

As said, I would prefer if modules are self-contained, all logic/code related to a module should be within a module.

Yes, async and Traits can be tricky, we refactor it into stand-alone functions first and when it works, we see how to put it into Traits or whether we just put the functions into their respective module. The abstractions will come ;)

@horenso
Copy link
Collaborator

horenso commented Aug 31, 2023

@markus2330 I have a few questions regarding config. Can we just have multiple copies of config cloating around, or should there only be one config object that is atomic? what about sync, is that done by libelectra?

1 similar comment
@horenso
Copy link
Collaborator

horenso commented Aug 31, 2023

@markus2330 I have a few questions regarding config. Can we just have multiple copies of config cloating around, or should there only be one config object that is atomic? what about sync, is that done by libelectra?

@markus2330
Copy link
Contributor

I have a few questions regarding config. Can we just have multiple copies of config cloating around,

Elektra's core definitely allows it but there are some plugins in Elektra that don't like concurrency. E.g. ElektraInitiative/libelektra#4466 was a problem for Opensesame but it should be fixed now. Furthermore, there might be issues if there are way too many open handles: ElektraInitiative/libelektra#3140

or should there only be one config object that is atomic?

Yeah, some mutex protection or similar is probably the simpler and safer way to use Elektra.

what about sync, is that done by libelectra?

Which sync do you mean? Sync on file system level is done by libelektra.

@markus2330
Copy link
Contributor

Elektra's core definitely allows it

To clarify: it is allowed (with potential limitations described above) to have separated instances of KDB, Key and KeySet per thread and use them concurrently.

src/audio.rs Outdated Show resolved Hide resolved
@fel115 fel115 marked this pull request as ready for review September 29, 2023 09:19
@fel115
Copy link
Collaborator Author

fel115 commented Sep 29, 2023

I think the refactoring to async is done. Other changes can be done in a separate PR. I already tested Buttons, Bell, PWR, Environment, Battery, Watchdog, Audio Bell, Garage, Signal, and Sensors. Today I am going to test Weatherstation in combination with other modules.

Which version of opensesame should this PR be? v0.8.0?

@horenso @markus2330

@markus2330
Copy link
Contributor

I think the refactoring to async is done. Other changes can be done in a separate PR. I already tested Buttons, Bell, PWR, Environment, Battery, Watchdog, Audio Bell, Garage, Signal, and Sensors.

Great! 🚀

Is it already ready to review? How to start reviewing this big PR, can you write some intro to the module system and how everything works together?

Today I am going to test Weatherstation in combination with other modules.

Please watch out that the weatherstation keeps on publishing most of the time. (If you install a broken version, please replace it with a working version again afterwards.)

Which version of opensesame should this PR be? v0.8.0?

Yes, at least if already everything works. Maybe even 0.9.0 to indicate we will be soon at 1.0.0?

@fel115
Copy link
Collaborator Author

fel115 commented Sep 29, 2023

Is it already ready to review? How to start reviewing this big PR, can you write some intro to the module system and how everything works together?

Yes, now it's ready for review. I hope my explanation below is helping.

OpenSesame

audio.rs

This module handles audio output for playing fire alarms and bell sounds. This module can receive commands from the Module Buttons (Bell), Environment (FireAlarm), Nextcloud (FireAlarm, Bell), and Signals (FireAlarm, Bell).

bat.rs

Checks the battery capacity every ten minutes and outputs to Nextcloud if it falls below 50%. If it goes below 50%, the threshold for the next Nextcloud message is set to 40%.

buttons.rs

This module implements the do_reset if an error occurs on the MOD-IO2 modules. In the async get_background_task, we implemented how the buttons were handled in the old main, including the validator and command receiver. The command receiver receives commands like opendoor (used by Garage, Nextcloud), ring_bell (used by Environment, Signals), switchlights (used by Garage, Nextcloud), and RingBellAlarm (used by Signals).

clima_sensor_us.rs

This module works independently, sending warnings to Nextcloud and publishing to opensensemap. We needed to implement Send to use libmodbus with async functions.

config.rs

No changes were made to this module.

environment.rs

Changed the functions rememberBaseline, restoreBaseline, and added a Muext of state. We moved handle_environment from the main into the get_background_task. This module receives commands (RestoreBaseline, RememberBaseline) from Signals.

garage.rs

This module is checked at intervals of 10 milliseconds, triggering Nextcloud Chat or Buttons commands if the button is pressed. Future changes will involve removing the interval and implementing trigger-oriented events because GPIO pins from the Olimex board are used, along with interrupts.

mod_ir_temp.rs

This module is triggered at given intervals and warns by sending Nextcloud messages.

nextcloud.rs

Implements two loops: one for sending (message_sender_loop) messages and status to Nextcloud, and the other for receiving (command_loop) messages/commands from Nextcloud. Commands can be sent via Nextcloud chat by typing "\opensesame" to open the door, or other commands like "\ring_bell", "\fire_alarm", "\status", and "\switchlights true true".

ping.rs

This module sends a ping message to Nextcloud if it receives the SendPing event. Other functions update Env, EnvStatus, EnvError, and BatCapacity, but these commands aren't used yet. They need to be triggered at the right spot in Environment and Battery to keep the ping up to date.

pwr.rs

No significant changes were made to this module.

sensors.rs

No major changes, only the get_background_task was added with a similar implementation as in the old version.

signals.rs

This module listens to system signals and executes the same events as in the old version.

ssh.rs

Changed to an async function.

types.rs

This module contains the error types defined so far.

validator.rs

No changes were made to this module.

watchdog.rs

This background task is triggered every few seconds and writes to the specified file. The test_get_background_task function is used to simulate the watchdog stopping writing to the watchdog file.

main.rs

In this loop, we initialize every module if it is enabled. Additionally, the MPSC (Multiple Producer, Single Consumer) channels are initialized here. To configure each module without encountering issues related to multiple access to one config object, we read the configuration in the main.

Copy link
Contributor

@markus2330 markus2330 left a comment

Choose a reason for hiding this comment

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

Great progress, I like many of these changes. The documentation is a bit poor, please describe what works now for all the modules. There are a few German comments. Some TODOs are not clear. TODOs shouldn't be bugs in code (for that please create issues) but only things that could be improved.

doc/Testreport_Async_Refactor.md Show resolved Hide resolved
doc/modules.md Outdated Show resolved Hide resolved
emu_dev/emulate.sh Outdated Show resolved Hide resolved
files/opensesame.spec Show resolved Hide resolved
files/opensesame.spec Outdated Show resolved Hide resolved
src/mod_ir_temp.rs Outdated Show resolved Hide resolved
src/nextcloud.rs Show resolved Hide resolved
src/signals.rs Show resolved Hide resolved
src/signals.rs Outdated Show resolved Hide resolved
src/signals.rs Outdated Show resolved Hide resolved
fel115 added 2 commits October 2, 2023 16:10
Because it isn't working, and wasn't implemented in the old version of opensesame
Copy link
Contributor

@markus2330 markus2330 left a comment

Choose a reason for hiding this comment

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

(only a few answers, did not review again)

src/sensors.rs Outdated Show resolved Hide resolved
src/nextcloud.rs Show resolved Hide resolved
src/signals.rs Show resolved Hide resolved
src/main.rs Show resolved Hide resolved
@fel115 fel115 requested review from markus2330 and horenso October 13, 2023 18:25
Copy link
Contributor

@markus2330 markus2330 left a comment

Choose a reason for hiding this comment

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

A few last issues are to be created and a few TODOs should refer to these issues, then we can merge this. I could test this tomorrow if we manage to merge it till then.

doc/Testreport_Async_Refactor.md Outdated Show resolved Hide resolved
doc/Testreport_Async_Refactor.md Outdated Show resolved Hide resolved
doc/Testreport_Async_Refactor.md Show resolved Hide resolved
doc/Testreport_Async_Refactor.md Outdated Show resolved Hide resolved
doc/modules.md Outdated Show resolved Hide resolved
doc/modules.md Outdated Show resolved Hide resolved
doc/modules.md Outdated Show resolved Hide resolved
src/buttons.rs Outdated Show resolved Hide resolved
src/buttons.rs Show resolved Hide resolved
src/sensors.rs Show resolved Hide resolved
@fel115
Copy link
Collaborator Author

fel115 commented Oct 14, 2023

I merged this branch with the main, and it is now ready. I can't merge into this repo @markus2330.

@markus2330 markus2330 merged commit 88f9d37 into ElektraInitiative:master Oct 14, 2023
@markus2330
Copy link
Contributor

Finally done 🚀 ❤️

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 this pull request may close these issues.

3 participants