RSDK 889 - Restart web service on media change #1793
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
viambot
added
safe to test
bhaney
requested changes
Jan 23, 2023
There was a problem hiding this comment.
Nice! This will be nice for people using RDK completely locally on their machine :)
One thing you will want to do is add a test in config/diff_test.go called TestDiffMedia(t *testing.T) that checks to see if camera/audio_input components being added/removed are picked up. You can use the diff tests in that file as templates.
Comment on lines
+410
to
+419
| for idx := 0; idx < len(added.Components); idx++ { | ||
| if added.Components[idx].Type == "camera" || added.Components[idx].Type == "audio_input" { | ||
| return true | ||
| } | ||
| } | ||
| for idx := 0; idx < len(removed.Components); idx++ { | ||
| if removed.Components[idx].Type == "camera" || removed.Components[idx].Type == "audio_input" { | ||
| return true | ||
| } | ||
| } |
There was a problem hiding this comment.
rather than using the strings "camera" and "audio_input", use the defined variables from the packages, i.e. camera.SubtypeName and audioinput.SubtypeName
There was a problem hiding this comment.
Unfortunately, I am not able to pull the camera and audioinput components into the config package due to a circular dependency (components also use config package).
I investigated passing those components into the DiffConfigs function directly but did not like how this turned out as it is used robotimpl which does not currently have access to the components.
I am thinking that is best to leave this hardcoded for now. Another option is to put a one-off config checker directly in the entrypoint.
There was a problem hiding this comment.
@edaniels what do you think about this?
There was a problem hiding this comment.
id file a ticket to address that and move on
There was a problem hiding this comment.
@bhaney added test for diffMedia
| if !diff.NetworkEqual { | ||
|
|
||
| if !diff.NetworkEqual || !diff.MediaEqual { | ||
| if err := myRobot.StopWeb(); err != nil { |
There was a problem hiding this comment.
I will let someone else comment about changing the order in which StopWeb() is called
edaniels
approved these changes
Jan 23, 2023
viambot
added
safe to test
|
Note: merge base coverage results not available, comparing against closest aa1914d~2=(9e2f63d) instead
|
|
Be sure to make a JIRA ticket for the reason why you have to hardcode the camera and auto_input strings, and can't use their SubType names |
viambot
added
safe to test
Closed
randhid
pushed a commit
to randhid/rdk
that referenced
this pull request
Feb 25, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR fixes a problem with the local
rc pagenot being able to switch fromgrpctowebrtcwhen a media component is added.Added a method to
diff.gocalleddiffMediaand an attribute to theDiffstruct calledMediaEqual.WARNING
I had to slightly rearrange the reconfiguring process to first
StopWebbeforecreateWebOptionsandmyRobot.Reconfigureto avoid race conditions with thewebServiceclose out.Test
To recreate, start with an empty config and run the server.
Notice how the rc page on
localhost:8080is using GRPC.Then, edit the config by adding a camera component:
Refresh the webpage - now loading with WebRTC!