Skip to content
This repository has been archived by the owner on Sep 8, 2024. It is now read-only.

Refactor - common bus connection method #2792

Merged
merged 6 commits into from
Jan 18, 2021
Merged

Conversation

krisgesling
Copy link
Contributor

Description

Following on from #2758 this switches the remaining Services to use the new common method for connecting to the message bus. This should only simplify the current code bringing together 4 different ways of doing the same thing.

How to test

Run Mycroft and ensure all Services start and connect to the message bus as expected.

Given the significance of changes to the startup flow of Services, each should be checked carefully.

Contributor license agreement signed?

@devops-mycroft devops-mycroft added the CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) label Jan 5, 2021
@devops-mycroft
Copy link

Voight Kampff Integration Test Failed (Results).
Mycroft logs are also available: skills.log, audio.log, voice.log, bus.log, enclosure.log

Copy link
Collaborator

@forslund forslund left a comment

Choose a reason for hiding this comment

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

One tiny nitpick (missing argument in docstring) otherwise it looks good.
I wonder if we should consider moving the mycroft.services.util to mycroft.util.process_utils or move the contents of that file into mycroft.service since they seem heavily related.

mycroft/services/util.py Outdated Show resolved Hide resolved
@devops-mycroft
Copy link

Voight Kampff Integration Test Failed (Results).
Mycroft logs are also available: skills.log, audio.log, voice.log, bus.log, enclosure.log

@krisgesling
Copy link
Contributor Author

Yeah I started the new module as I originally thought of abstracting more of the duplicated code to ensure all the services operate consistently and explicitly continue to do so (maybe a Service base class?).

But process_utils looks like the perfect place for this right now.

Good spot on the docstring too :)

@devops-mycroft
Copy link

Voight Kampff Integration Test Failed (Results).
Mycroft logs are also available: skills.log, audio.log, voice.log, bus.log, enclosure.log

2 similar comments
@devops-mycroft
Copy link

Voight Kampff Integration Test Failed (Results).
Mycroft logs are also available: skills.log, audio.log, voice.log, bus.log, enclosure.log

@devops-mycroft
Copy link

Voight Kampff Integration Test Failed (Results).
Mycroft logs are also available: skills.log, audio.log, voice.log, bus.log, enclosure.log

forslund
forslund previously approved these changes Jan 10, 2021
@forslund
Copy link
Collaborator

Very nice, tested and working. 👍

@devops-mycroft
Copy link

Voight Kampff Integration Test Failed (Results).
Mycroft logs are also available: skills.log, audio.log, voice.log, bus.log, enclosure.log

2 similar comments
@devops-mycroft
Copy link

Voight Kampff Integration Test Failed (Results).
Mycroft logs are also available: skills.log, audio.log, voice.log, bus.log, enclosure.log

@devops-mycroft
Copy link

Voight Kampff Integration Test Failed (Results).
Mycroft logs are also available: skills.log, audio.log, voice.log, bus.log, enclosure.log

@devops-mycroft
Copy link

Voight Kampff Integration Test Failed (Results).
Mycroft logs are also available: skills.log, audio.log, voice.log, bus.log, enclosure.log

@JarbasAl
Copy link
Contributor

to avoid code duplication, i would argue this should go into https://github.com/MycroftAI/mycroft-messagebus-client/

and the whole client folder in mycroft deprecated, mycroft-core only needs to contain the server, the Message object and the MessagebusClient can / should be imported from package above

@krisgesling
Copy link
Contributor Author

I don't disagree with this in principle, means that everyone is using a common client rather than the client in core diverging from the client for 3rd parties. This PR still seems like it's going in the right direction toward that end goal.

@devops-mycroft
Copy link

Voight Kampff Integration Test Failed (Results).
Mycroft logs are also available: skills.log, audio.log, voice.log, bus.log, enclosure.log

1 similar comment
@devops-mycroft
Copy link

Voight Kampff Integration Test Failed (Results).
Mycroft logs are also available: skills.log, audio.log, voice.log, bus.log, enclosure.log

Copy link
Collaborator

@forslund forslund left a comment

Choose a reason for hiding this comment

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

Finally realized the issue here... The audioservices aren't registered...

The bus handlers was set to be registered when the bus started in an event handler, see audioservice.pyL211. Can't quite recall why this was I believe it was a performance thing.

I think you should be able to fix it by replacing the line with

self.load_services()

and renaming the load_services_callback method to self.load_services

Tested locally before (failing) and after (succeeding) this commit

@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

@forslund forslund force-pushed the feature/common-bus-connection branch from ddfaf68 to 375adbb Compare January 15, 2021 13:08
@devops-mycroft
Copy link

Voight Kampff Integration Test Failed (Results).
Mycroft logs are also available: skills.log, audio.log, voice.log, bus.log, enclosure.log

@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

@krisgesling krisgesling merged commit 303f6a3 into dev Jan 18, 2021
@krisgesling krisgesling deleted the feature/common-bus-connection branch January 18, 2021 06:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) Status: Change requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants