Closed theresalech closed 2 years ago
Url
parameter? Through a websocket? I think there's a lack of definition about the actual plugin interface.Each of these plugins would maintain their own connection to SDL Core using a router service, which can be borrowed from the Manticore project (see here).
I can definitely add a section which describes this is more detail. The message broker would first connect to Core and all of the plugins (plus the main HMI) would separately connect to this application. The message broker would then direct any messages from Core to the appropriate plugins based on the interfaces they registered (VR, UI, etc.). The messages which are sent between plugins will need their own interface, but we should be able to route these messages in a similar way, since all of the plugins are connected to the message broker:
These messages can be routed using the message broker as well. Each message would include the
Plugin
prefix and can include adestination
parameter if targeting a specific component.
Url
parameter and displayed via iframes
or something similar.I can try to update the description for these features make this more clear. I think better terminology would definitely help with this, so I'm open to any suggestions on how to differentiate them better.
The Steering Committee voted to keep this proposal in review, to allow for further discussion on the review issue.
1. Okay
2. Okay, I think this needs to be fleshed out in the proposal. We also need a list of all plugin "types," and their supported HMI RPCs. Please let me know if that doesn't make sense in the context of the proposal.
3. For part 2, production plugins, how flexible is this? Given that it's for production plugins, I would think it would need to be extremely flexible. For example, having a slide-out hamburger menu is probably not good UX design for a vehicle, as it has a small tap-target and hidden UI is generally not something that should be used. So I think (1) we have to make sure anything like this for production plugins is very customizable, (2) have a better default than a slide-out menu.
2.
We also need a list of all plugin "types," and their supported HMI RPCs
I believe this is already covered by the Example Plugins
section. Although I was leaving this list open to expansion, I could just make this the list of officially supported plugin types.
I believe there were plans to add this slide-out menu to the Generic HMI already in the works, which is why I used that example, but if we want to pivot the design for this we can. Would something like the TILES
menu layout be a better default entry point? Obviously I'm open to any other suggestions as well.
The Steering Committee voted to keep this proposal in review, to allow for further discussion on the review issue.
Based on the discussion here, the Project Maintainer recommends the Steering Committee return this proposal for the following revisions:
Example Plugins
section the list of officially supported plugin types.The Steering Committee voted to return this proposal for the revisions described in this comment.
Closing as inactive. The issue will remain in a returned for revisions
state and unlocked so the author can notify the Project Maintainer when revisions have been submitted.
The author has revised this proposal based on the Steering Committee's requested revisions, and the revised proposal is now in review until October 26, 2021.
Unfortunately, with the releases tomorrow I have not been able to re-review this proposal. I would like to request an additional week to review.
The Steering Committee voted to keep this proposal in review for an additional week.
1. Under "Plugin Configuration", you state:
It should be possible to add, remove, disable, and replace these plugins with minimal configuration changes where feasible. Ideally, this configuration could be handled using the existing Flags.js file, which is currently used by the Generic HMI for any runtime configurations. A potential format for this configuration could be:...Note: This format is tentative and could change depending on the final implementation.
This language does a lot of hedging here, and because this is the proposed solution of a public API, I don't think you can hedge like this. The language needs to be changed to "will be" instead of "could be" for example. We can't have a "potential" format, we need "the format." If needed during development, we can change them through proposal revisions, but proposals are the spec. So either updates need to be made to make a spec, or the language needs to be updated.
2. Under "Web-based plugins":
In order to run these applications within the browser, the Generic HMI could open the provided Url in the plugin configuration within a hidden iframe or something similar, allowing the plugin to run alongside the HMI.
Similar here, I think this needs to be changed to, "...the Generic HMI will open the provided Url in the plugin configuration within a hidden iframe or something similar, allowing the plugin to run alongside the HMI."
3. While I'm happy to see the plugin types changed from examples to the concrete types, I think they're still under-specced. You've listed the API names, but nothing about the API details. I think you need to include information like "how are these called" (JSON-RPC? You mentioned multiple language support so I assume it's not just JavaScript functions), the API input and output types, etc.
4. Under "App Service Types", you write:
This could potentially be split into several plugins (by service type) depending on the system.
Again, I think this "could potentially" language needs to be taken out unless it's under the "Alternatives" section. Give a firm spec that is implementable.
5. I'm a bit confused about the section "Messages between components". Is this intended to describe APIs that developer plugins would need to support for these different features to work? What if a developer implements the module but not these methods? How do they set up to respond to these methods? What do they respond with? You say these are "a few of the potential" ones, but that leaves the public API surface up in the air again. I would rather see us do what we have in the past, which is set up a public API surface in the proposal and adjust it through revisions later if needed.
You also write, "In addition, customized messages will likely need to be created for production systems to accommodate differences in plugin design."
What differences in plugin design are you think of?
6. Again in "Integrated Plugin Controls" there a lot of "possibility" and "could" language. Could you please change that into a firm proposal with "will" language?
7. In "Impact to existing code" you mention: "Overall, a majority of the changes needed for this proposal will be in the form of test plugins, which will be isolated from any existing code." Is it a part of this proposal to create example versions of some / all of the plugins mentioned? Will they be released to the generic_hmi repo? Other repos? Will there be documentation on how to use them?
@joeljfischer
I might still specifically argue that it makes more sense for the configuration file to be left as an implementation detail, as it is relatively internal to the HMI, but I could also see this factoring into points 2 and 6, since they use this configuration.
I think some of this "maybe" language is probably bleeding over from the configuration section. I can update the wording to be more concrete.
Sorry, I don't think I'm understanding where the confusion is here. These messages are defined in the HMI API, and the plugins would handle and/or send them the same way that the HMI does currently, they would just process a specific subset of RPCs. The programming language for the plugin would not factor into this.
For example, the VehicleInfo plugin could receive a VehicleInfo.GetVehicleData
request from Core:
{
"id":11,
"jsonrpc":"2.0",
"method":"VehicleInfo.GetVehicleData",
"params":{
"vin":true
}
}
And the plugin would respond to the message, just as the HMI would normally:
{
"jsonrpc":"2.0",
"id":11,
"result":{
"vin":"52-452-52-752",
"code":0,
"method":"VehicleInfo.GetVehicleData"
}
}
I disagree in this case, as this is meant to be a potential design decision for the plugin creator. The Generic HMI should be able to handle both cases (combined or separate app service plugins). If you think it is unclear, I could perhaps reword it to say that the HMI supports both of these designs.
Alright, I'll agree that this is probably the section most in need of a concrete specification. I'm fairly certain that this list will need to be expanded once development starts, which is why I'd originally included the open-ended language, but at this point it would likely be better to just update the proposal if there are any significant changes to this section.
As for the comment about "differences in plugin design", I didn't have anything specific in mind, but I wanted to make sure that the design was kept flexible enough so that an HMI developer could implement custom interactions if they needed them.
Same as 2
Yes, this is stated in the proposal
As part of this proposal, an example plugin of each type should be created for testing purposes, and these examples can then be used as a reference for production implementations.
My initial thought was that these would be included directly in the project, as this would allow users to have a fully functioning test HMI with minimal configuration, but they could be maintained in separate repositories if that would be preferred. Of course, the documentation would be updated accordingly (most likely this would be included in the README or the Core Guides section of the developer portal).
We did not have a quorum present during our meeting on 2021-11-02, and so were unable to vote on this proposal. It will therefore remain in review until our next meeting on 2021-11-09.
1. Yeah, I understand that a lot of this is hard to figure out ahead of time without doing all the work, but the rule has generally been that implementation details should be explained in detail if they're important and anything developer-facing must be set and changed through revisions.
The configuration does seem to be developer-facing. I can't see an implementation of this feature that doesn't require the developer to update and change the configuration. I think that would require us to call this a public API.
2. 👍
3. That's my mistake based on my unfamiliarity with the HMI API.
4. I understand now, thank you for clarifying. As long as the configuration allows for this decision, would the developer just put the same IP/Port combo for multiple plugin configurations?
5. 👍
6. 👍
7. I think including them in the repo is reasonable, or perhaps a separate monorepo included as a submodule. Either way should be specified though.
1. Alright, I can try to solidify the configuration file format.
4. Ah, another consequence of not having this defined in the configuration description. In this case I was thinking that the both the AppService and RemoteControl plugin sections of the config file could each accept an array of plugin configurations, with each array element having their own URL. I can add this to the configuration description.
7. 👍
To summarize the changes requested:
AppService
).The Steering Committee voted to return this proposal for the revisions outlined in this comment.
Closing as inactive. The issue will remain in a returned for revisions
state and unlocked so the author can notify the Project Maintainer when revisions have been submitted.
The author has revised this proposal based on the Steering Committee's requested revisions, and the revised proposal is now in review until January 18, 2022.
The specific items that were updated since the last review can be found in this merged pull request: #1182.
I apologize, but I would like more time to review this.
The Steering Committee voted to keep this proposal in review until our next meeting to allow for additional time for member and project maintainer review.
I believe that all of my concerns have been addressed.
The Steering Committee fully agreed to accept this proposal.
Implementation issue: https://github.com/smartdevicelink/generic_hmi/issues/496
Hello SDL community,
The review of the revised proposal "SDL 0341 - Add Generic HMI Plugin Support" begins now and runs through January 18, 2022. Previous reviews of the proposal took place October 20 - November 9, 2021, and July 21 - August 16, 2021. The proposal is available here:
https://github.com/smartdevicelink/sdl_evolution/blob/master/proposals/0341-add-generic-hmi-plugin-support.md
Reviews are an important part of the SDL evolution process. All reviews should be sent to the associated Github issue at:
https://github.com/smartdevicelink/sdl_evolution/issues/1168
What goes into a review?
The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of SDL. When writing your review, here are some questions you might want to answer in your review:
More information about the SDL evolution process is available at
https://github.com/smartdevicelink/sdl_evolution/blob/master/process.md
Thank you, Theresa Lech
Program Manager - Livio theresa@livio.io