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

DialogFlow APIv2 Integration (GoogleHome) - dev #4035

Closed
wants to merge 60 commits into from

Conversation

mdomox
Copy link
Contributor

@mdomox mdomox commented Oct 31, 2018

No description provided.

, intentHandler: googleHomeCurrentBasalhandler
}]
};

Copy link
Member

Choose a reason for hiding this comment

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

What API call changes are needed to have this live in the Home plugin? basalprofile.js should only contain basalprofile code.

, slots: ['iob', 'insulin on board', 'insulin']
, intentHandler: googleHomeIOBIntentHandler
}]
};
Copy link
Member

Choose a reason for hiding this comment

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

Same here let's aim to restructure both Home and Alexa so the IOB reporting code is in the plugin, not in iob.js

Copy link
Member

Choose a reason for hiding this comment

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

Alternatively, would it be possible to add a single method that output a string for speech and reuse that in both Home and Alexa? That'd maybe be the cleanest solution?


if (env.settings.isEnabled('googlehome')) {
ctx.googleHome = require('../plugins/googlehome')(env, ctx);
}

next( );
}
Copy link
Member

Choose a reason for hiding this comment

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

@PieterGit let's try to figure out if there's a way to reduce plugin-specific code in this section

intent: 'LastLoop'
, intentHandler: googleHomeLastLoopHandler
}]
};

Copy link
Member

Choose a reason for hiding this comment

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

And same here - Home and Alexa code for creating the speech user interface should be in the plugin itself. What'd be acceptable for this file is a generic API method that returns a string for speech, which is reused in both Home and Alexa speech implementations, but custom method for both adds a lot of maintenance burden

@PieterGit
Copy link
Contributor

PieterGit commented Nov 16, 2018

@mdomox can you fix @sulkaharo requested changes in the coming days or weeks. These changes should be made before the feature freeze for the 0.11 release. I'm thinking of postponing the freeze to beginning of december, so that we can release 0.11 before xmas.

Perhaps @rickfriele can give you some help. The issues raised by @sulkaharo must be resolved before we can merge this to dev. I also would like another Google Home user to confirm it works after you made the changes.

@mdomox
Copy link
Contributor Author

mdomox commented Nov 24, 2018

@mdomox can you fix @sulkaharo requested changes in the coming days or weeks. These changes should be made before the feature freeze for the 0.11 release. I'm thinking of postponing the freeze to beginning of december, so that we can release 0.11 before xmas.

Perhaps @rickfriele can give you some help. The issues raised by @sulkaharo must be resolved before we can merge this to dev. I also would like another Google Home user to confirm it works after you made the changes.

The issues raised are common with Alexa code. As I understood a restructure is needed. I will try, but I am not 100% confident I can manage it in the timeframe required (I am new to this ). Is there documentation regarding all the variables, objects and functions declared in this project? @PieterGit

This is a DialogFlow integration. It can interface to a number of endpoints (Telegram, Viber, Skype, Facebook Messenger, Siri, Google Assistant). GoogleHome in fact, is just another Google Assistant player (as your WearOS smartwatch and Android phone).

@RalfCGM
Copy link

RalfCGM commented Nov 27, 2018

Really can't help with programming, but would like to try to help testing with Google assistant. I don't quite understand if there some code ready to be tested, but if somebody could confirm and help with merging this pull request in my ns, I'm all in.

@PieterGit
Copy link
Contributor

@mdomox I postponed this to 0.12 release. This needs better integration. Please ask @sulkaharo for help if you need help. See https://gitter.im/nightscout/public?at=5c093bdf178d7860a194d359

@PieterGit
Copy link
Contributor

@mdomox can you give an update if you have time to work on better Google Home (and maybe also Alexa ) integration? See also this discussion which is relevant for Google Home integration as well: #4168 (comment)

@mdomox
Copy link
Contributor Author

mdomox commented Jan 6, 2019

@mdomox can you give an update if you have time to work on better Google Home (and maybe also Alexa ) integration? See also this discussion which is relevant for Google Home integration as well: #4168 (comment)
@PieterGit I am not sure this can be done, since dialogflow integration relies on Google's speech API. https://dialogflow.com/docs/reference/api-v2/rest/v2/projects.agent.intents

@mdomox
Copy link
Contributor Author

mdomox commented Jan 6, 2019

@mdomox can you give an update if you have time to work on better Google Home (and maybe also Alexa ) integration? See also this discussion which is relevant for Google Home integration as well: #4168 (comment)
@PieterGit I am not sure this can be done, since dialogflow integration relies on Google's speech API. https://dialogflow.com/docs/reference/api-v2/rest/v2/projects.agent.intents

@PieterGit , moreover, now it runs independently, having the speech plugin disabled, since it constantly produces output on every new BG value (something that is not necessary or even annoying). The current implementation is not intrusive, meaning that it does not produce output unless called by the user.

Copy link
Contributor

@PieterGit PieterGit left a comment

Choose a reason for hiding this comment

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

@mdomox I did a code review. Thanks for making it more run independently. I found the following issues with this PR:

lib/api/googlehome/index.js
line 10: 'translate' is assigned a value but never used. (no-unused-vars)
var translate = ctx.language.translate;
lib/plugins/googlehome.js
line 2: 'ctx' is defined but never used. (no-unused-vars)
lib/plugins/basalprofile.js
lib/plugins/cob.js
lib/plugins/iob.js
lib/plugins/openaps.js

See https://github.com/nightscout/cgm-remote-monitor/pull/4035/files for details.

  • Can you add unit tests for the plugin?
  • Can you add swagger docs for the /googlehome api?

@mdomox
Copy link
Contributor Author

mdomox commented Feb 20, 2019

@mdomox I did a code review. Thanks for making it more run independently. I found the following issues with this PR:

lib/api/googlehome/index.js
line 10: 'translate' is assigned a value but never used. (no-unused-vars)
var translate = ctx.language.translate;
lib/plugins/googlehome.js
line 2: 'ctx' is defined but never used. (no-unused-vars)
lib/plugins/basalprofile.js
lib/plugins/cob.js
lib/plugins/iob.js
lib/plugins/openaps.js

See https://github.com/nightscout/cgm-remote-monitor/pull/4035/files for details.

  • Can you add unit tests for the plugin?
  • Can you add swagger docs for the /googlehome api?

@PieterGit
1st comment : this is work in progress in an efort to use ctx translations. Will be removed-cleaned.

2nd comment: intentHandlers in those core plugins already exist for Alexa . Why not for DialogFlow? Why is this a show-stopper?

3rd comment: How is this done? Is there a documentation on how to perform those tests?

4rd comment: A doc is in the /doc folder. I wil review it and maybe update it.

@jamieowendexcom
Copy link

I have tested the mdomox googlehome on Dialogflow and my Googlehome.
All working fine and no issues so far.
Thanks to @mdomox for creating the code.

@jamieowendexcom
Copy link

Are we any closer to getting this merged into the master?

@sulkaharo
Copy link
Member

Probably not, given lack of activity in the last few months to address the improvement requests above

@nielsmaerten
Copy link

For what it's worth: I may have an alternative to this.
I've been working on my own Google Assistant + Nightscout integration for a few months now:
Nightscout Status
At time of writing, around 200 people are using it daily and it's pretty reliable.

I don't mean to diminish the great work @mdomox has been doing and I realize this is not a full alternative. There will always be people who prefer to set up their own DialogFlow Actions. But for those of us who don't feel comfortable doing that, I thought a SaaS might be useful.
(Kinda like running Autotune yourself vs. AutotuneWeb.)

@inventor96
Copy link
Contributor

@sulkaharo @PieterGit I'm willing to take a stab at updating this PR (as I have time) to resolve the issues brought up. I'm a full-stack web developer, but unfortunately Node is not a platform I have experience with, so any pointers you can give to help me get started would be great!

inventor96 referenced this pull request in inventor96/cgm-remote-monitor Sep 6, 2019
@PieterGit
Copy link
Contributor

Closing this PR. It will be integrated with the product with #4980

@PieterGit PieterGit closed this Oct 18, 2019
@PieterGit PieterGit removed this from the 0.12.0 milestone Oct 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants