Yio Remote Community

Ideas for remote implementation

I misunderstood @ChristianRiedl.

Makes sense to have the connected state in the entity. It would make the UI also more self contained.

@marton

Newest Changes in feature/weather

Unfortunately there are now a lot of changes in the feature/weather branch. So here some explanations:

In general : replacing unnecessary QObject* to avoid qobject_cast* and cleaning up the whole structure.

Integrations

The Integration.h should be used only inside the integration itself.
To access the integration from C++ inside the remote software (entities) a interface IntegrationInterface is required.
Unfortunately this name was used as interface for the integration plugin (= instance factory)

  • So the old IntegrationInterface is renamed to PluginInterface.
  • As consequence Logger and Integrations must be changed to use PluginInterface (logger., integrations., integrationsinterface.h)
  • The renaming must be done also in the integrations of course.
  • remove unnecessary integration.h include in main.cpp
  • sendCommand is now part of IntegrationInterface and Integration in the integration it must be overriden, not a slot.
  • the IntegrationInterface destrcutor must be added somewhere in the integrations .cpp.
  • the Integration::setup function must be called from the integrators setup. It initializes a m_entities member which can be used also in the integration.

Entities

  • Changing the integrationObj QObject* to IntegrationInterface*. No more accessible from QML.
    command function can now use the IntegrationInterface instead of InvokeMethod.
    (The integration is still QML accessible from integration). (entity.* and entityinterface.h)

  • As conseqeuence all the concrete entity constructors (blind, light, remote) must be changed to use IntegrationInterface instead of QObject.
    (blind., light., weather., remote., mediaplayer.*)

  • Adding a connected attribute to Entity class. The state is set from the Integration class when state changes.
    The entity emits only a signal ConnectedChanged. (entity.* and entityinterface.h, integration.h,
    components : ButtonBase.qml, Button.qm (light, blind, weather))

  • Adding setConnected(entiId) to EntitiesInterface and Entities. Change entity from QObject* to Entity*.
    As consequence the getBy* functions can be optimized not using slow getProperty. (EntitiesInterface.h, Entities.*)

Mediaplayer

  • The browse/search results are delivered from the integration as QAbtractListModel, optimized for ListView. The old browseResult and a new model property is introduzed instead.
    (mediaplayer.*, mediaplayerinterface.h)

Dock integration

  • remoteinterface.h was added to allow access to the remote entity commands attribute. (remote.*)

Older changes in feature/weather

  • Adding weather entity including QML components and icons
  • New Logger

My Merge Support (integrations)

I have tested the new version not only with “my” integrations. I tested it with home-assistant and
dock integration (as far it is possible without hardware). I will make the changes also in homey integration even if I cannot test it.

So I applied some changes for this not “my” integrations and will make them available as feature/christianriedl.

Marton, if you want this integration changes, Please create branches for me, I have not eonough rights. (integration homey, home-assistant, dock)

I hope you agree with my approach. But I feel a bit guilty because of my obsession with change. Slowly, the whole thing looks like I imagine this and I will then hold back with changes.

I changed :

  • Renaming to PluginInterface
  • Added category logging
  • Added my new and faster updateByAttrIndex function
  • Made some performance improvements by
    • using new available functions like entity.isSupported(feature)
    • avoiding repeated access into the same QVariantMap structure

I think you have the right @ChristianRiedl to create branches, but I have created a feature branch in all integration repos for you.

I don’t mind these changes. It makes sense to clean these things up. I am making some changes in the feature/mediabrowsing branch while working on the Spotify integration and creating all the UI elements needed for search and browsing. I’d like to finish it and then merge your code to the dev branch. There will be a lot of conflict I guess, because these are “breaking changes”, so I would tackle that a bit later (around end of next week).

@marton

After the experience writing my first “real” integration (openHab) I see something which should be improved :slight_smile:

We introduce attribute enum because of speed. On the command side speed is not so important.

But it would be better to use enums also for the supported_features and commands.

Strings are spongy inside a program they should not be used I think. Only for things which must be read by humans. Every comparison with enums is hundred times faster than a string compare. And enums are intellisense friendly. And the conversion enum <-> string we get automatically from Qt. And a C++ switch is more readable than a if else if else if chain.

I know I promised to keep quiet, and it is nor very urgent, but maybe now is a good chance to do it. If you give me a go, I change it in the entities and the integrations.

@ChristianRiedl sure, do it :slight_smile:

Would be great if you could put it in the same branch so I can merge it with the rest of the stuff.
I hope to merge it this weekend and will also clean around in the repos, so everyone who is pushing code will be on the same page.

After that, we can add more and more code to it :slight_smile:

@marton @Nklerk

I finished the change to enums for commans and features.

I pushed the changes to

  • remote_software/feature_weather,
  • integration.openweather/develop
  • integration.openhab/dev
  • integration.roon/develop
  • integration.home-assistant/feature/interface_update
  • integration.homey/feature/interface_update

I also made the changes for integration.dock. When you create a feature branch I can push it.

integration.dock also handles dock commands, for this purpose I introduced in remoteinterface.h

    // Not entity related but must be defined somewhere
    C_REMOTE_CHARGED, C_REMOTE_LOWBATTERY,

In main.qml

 import Entity.Remote 1.0            // @@@RIC
 
 //@@@RIC obj.sendCommand("dock", "", "REMOTE_CHARGED", "");
 obj.sendCommand("dock", "", Remote.C_REMOTE_CHARGED, "");

Please change all supported_feature[“XXX”] >= 0 against isSupported (Light.F_XXX") in the qml files !!!

For MediaPlayer I have to change the playMedia function because command is now an integer.
We pack key and play command in a structure (QVariantMap). I also added a search_item for context search.

void MediaPlayer::playMedia(const QString& cmd, const QString& itemKey)
{
    QVariantMap ccmd;
    ccmd["key"] = itemKey;
    ccmd["command"] = cmd;
    command(MediaPlayerDef::C_PLAY_ITEM, ccmd);
}
void MediaPlayer::searchItem(const QString &searchText, const QString &itemKey)
{
    QVariantMap ccmd;
    ccmd["key"] = itemKey;
    ccmd["text"] = searchText;
    command(MediaPlayerDef::C_SEARCH_ITEM, ccmd);
}

Thanks @ChristianRiedl!

Could you look at the various integrations? I can see there are some conflicts when creating a PR.

Yesterday, I worked on the media player. Will push the updates soon to the feature/mediabrowsing branch. I suggest let’s wait with changes to the mediaplayer, because I am pushing a lot.

@marton @zehnm

Now I pushed the changes for dock and home-assistant (once more) to feature branch interface-update.

Uploading via github webpage creates problems with line endings ?

Hey @ChristianRiedl! I have merged all the repos except the remote-software. I’m holding back from this one until I’ve pushed all the changes. Then I’ll replace all the supported_feature things with isSupported.

@marton

OK. I don’t need a copyright line when I change string to enum in other contributors sources.
My motivation was only to push through breaking changes on which I am convinced and to accelerate the whole process :grinning:. I am satisfied when people agree with the “improvements”.

1 Like

You have been contributing a lot and crucial parts of the code so I think it’s fine you have that :slight_smile:
I’m hoping to merge everything this weekend, including the media browsing I am working on. So far it works quite good. Can’t wait to show it!

@ChristianRiedl

I have been thinking about moving the integrations to a separate thread when they are created in the plugin factory. That way we could skip the thread creation inside the integration and we could secure that all integrations run in a thread, therefore not blocking the UI. What do you think about this? Do you think this could work and help with performance?

@marton

I have an idea of a more powerful Plugin and Integration class, which handles the features common to all integrations, including creation of the separate thread. I dislike that every integration implements this feature individually.

But in general we should be careful creating more and more threads. It is a single core CPU, and it is great that Qt has the powerfull signal/slot mechanism working across threads. But crossing the thread boundaries is always costing a lot CPU. When we shift too much into separate threads we can get a problem too.

For websocket based integrations I googled an explanation, that receiving via websocket is not really asynchronous and causes some blocking. Maybe it is a kind of bug which will be fixed in Qt in future. Maybe polling integrations using REST with QNetWorkAccessManager do not have this problem. The integration class I am thinking about will allow to easily switch between using a separate thread or not.

@ChristianRiedl

Great. Would you mind sharing more of how you imagined it? Or if it’s something more concrete, please open a feature branch.

I don’t want to go overboard with threads, just want to make sure the UI stays fluid and responsive. Unfortunately the websocket implementation in Qt is buggy. It should be async, but it’s not. QNetworkAccessManager runs in its own thread as far as I know. I have to do more testing with the Spotify integration to double check.

@marton

I will create a prototype of the home-assistant integration in a feature branch based on my ideas. Than it is easier to discuss it.

Anyhow when a thread is required then we need 2 integration classes, like in your current home-assistant integration :

One class running on the UI thread, receiving the sendCommand, connect, disconnect … calls delegating via signal /slot to the thread class and one worker thread class performing I/O and updating the entities.

I think it is not necessary that thread class delegates setState back to UI thread, but i have to test it.

Anyhow I will encapsulate all this in one and only Integration base class handling all this stuff for all integrations in a generic way. I know service detection in dock integration requires special handling so it is neccary to override the standard behavior.

I believe this is the right moment to start with a static integration library. I can start it but maybe somebody must help me to create a .pro file considering the whole project environment. It is really not nice that some .cpp files which are only used by the integrations are living inside the remote project.

@ChristianRiedl Thanks!

My idea for this is very simple. Just adding these to lines to Integrations::add
QThread *thread = new QThread(this);
obj->moveToThread(thread);
thread->start();

void Integrations::add(const QVariantMap& config, QObject *obj, const QString& type)
{   
    QThread *thread = new QThread(this);
    obj->moveToThread(thread);
    thread->start();

    m_integrations.insert(config.value("id").toString(), obj);
    m_integrations_friendly_names.insert(config.value("id").toString(), config.value("friendly_name").toString());
    m_integrations_mdns.insert(config.value("id").toString(), config.value("mdns").toString());
    m_integrations_types.insert(config.value("id").toString(), type);
    emit listChanged();
}

This way it is independent from the integrations. What do you think?

@marton

Then every integration would run on a separate thread and calls like connect, sendCommand must be done via emit signal. I wanted to avoid that every integration must run in a thread. Currently I believe, that only QWebSockets causes the problem. What is your experience with the Spotify integration, does it have negative influence on the UI ?

I have now a working solution which is similar than old solution but the Integration class creates an intermediate “proxy” object if a separate thread is required which translates the connect, sendcommand calls to signals.

Its easy to decide : When REST based integrations are working well without threads than we should use my solution, if we find out that all kind of integrations require a thread than we should follow your idea.

Anyhow I will push my solution to the home-assistant feature branch, and you can take a look to my implementation.

@ChristianRiedl I am not sure which solution is better. Your proposal sounds good that not every integration runs in a thread. The Spotify integration is not blocking the UI that much. There can be some hiccups but that is I guess due to the amount of images that are loading. Maybe a custom image loader that runs in a thread could be a solution for that.

Please do push your solution to github. I’d like to give it a go!

@marton
I pushed my solution to integration.home-assistant branch feature/integration-framework.

The integration directory is only temporary, we can either put it to the remotes integration directory or to an integration library, together with other things (QAbstractListModel) needed only by integrations.

Look to the homeassistant.h, it streamlines integration development. For fast testing you can set the need for a worker thread in the config.json.

From the performance perspective there will be no difference between the 2 solutions, because essentially they do the same. My proposal requires one function call more to emit a sendcommand signal. Of course I vote for my solution as it gives us the flexibility to choose easily between workerthread ot not :grinning: .

@marton

How shall I continue ? Do you want to use my proposal ? I propose to start also the integration-library now containing all sources used only by integrations. If you agree please create a repository for the library with a dev branch.