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

Video: Send VIDEO_STREAM_INFORMATION to GCS #201

Merged
merged 1 commit into from
Mar 13, 2024

Conversation

ddd999
Copy link

@ddd999 ddd999 commented Feb 7, 2024

Addresses feature request issue #169

Enables Rpanion to respond to send COMMAND_ACK and VIDEO_STREAM_INFORMATION messages via MavLink in response to a MAV_CMD_REQUEST_MESSAGE.

@ddd999 ddd999 marked this pull request as draft February 8, 2024 02:20
@ddd999
Copy link
Author

ddd999 commented Feb 8, 2024

Converting to draft because I realized it's going to be mandatory to have a way to select the network interface and therefore, which URI gets sent in the VIDEO_STREAM_INFORMATION message. The only way to send multiple URIs would be to send multiple messages (i.e., one to every interface), which seems kludgy.

This would work fine if the URI was hardcoded to one interface (or all interfaces), but there are plenty of use cases where a user would want to choose.

@ddd999 ddd999 marked this pull request as ready for review February 8, 2024 07:26
@ddd999 ddd999 marked this pull request as draft February 8, 2024 07:26
@stephendade
Copy link
Owner

Nice! When you're ready, happy to give it test and review.

@ddd999 ddd999 force-pushed the videostreaminfo branch 4 times, most recently from 5eb591d to 0b1d961 Compare March 3, 2024 00:48
@ddd999 ddd999 marked this pull request as ready for review March 3, 2024 00:54
@ddd999
Copy link
Author

ddd999 commented Mar 3, 2024

@stephendade, please review when you have time. Thank you!

Thanks to @TyIsI for helping with tips to get this working as well.

@stephendade
Copy link
Owner

Great! Looks like you'll need a rebase first, as some of my recent tlogging changes aren't in here.

@ddd999 ddd999 force-pushed the videostreaminfo branch 2 times, most recently from a287ac6 to 47ddedf Compare March 6, 2024 02:50
@ddd999 ddd999 closed this Mar 7, 2024
@ddd999 ddd999 force-pushed the videostreaminfo branch from 47ddedf to 1564a64 Compare March 7, 2024 03:00
@ddd999 ddd999 reopened this Mar 7, 2024
@ddd999
Copy link
Author

ddd999 commented Mar 7, 2024

@stephendade I had a bit of fight with git but I believe it is all up to date now.

@stephendade
Copy link
Owner

Yes, that look up to date now. I should have some time in the next few days to review.

Copy link
Owner

@stephendade stephendade left a comment

Choose a reason for hiding this comment

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

Looks pretty good. Some minor formatting issues and the user needs a bit more information about the service.

src/video.js Outdated
@@ -186,6 +194,12 @@ class VideoPage extends basePage {
<input disabled={this.state.streamingStatus} type="number" name="fps" min="1" max={this.state.FPSMax} step="1" onChange={this.handleFPSChange} value={this.state.fpsSelected} />fps (max: {this.state.FPSMax})
</div>
</div>
<div className="form-group row" style={{ marginBottom: '5px' }}>
Copy link
Owner

Choose a reason for hiding this comment

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

    <br />
    <h3>MAVLink Video Streaming Service</h3>
    <p><i>Configuration for advertising the video stream via MAVLink. See <a href='https://mavlink.io/en/services/camera.html#video_streaming'>here</a> for details.</i></p>
    <div className="form-group row" style={{ marginBottom: '5px' }}>
      <label className="col-sm-4 col-form-label">Video source IP Address</label>
      <div className="col-sm-8">
        <Select isDisabled={this.state.streamingStatus} onChange={this.handleMavStreamChange} options={this.state.ifaces.map((item, index) => ({ value: index, label: item}))} value={this.state.mavStreamSelected} />
      </div>
    </div>

Copy link
Owner

Choose a reason for hiding this comment

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

Also needs the "Stream Configuration" header changed to h3 to match here

} else if ( data.targetSystem === this.targetSystem &&
data.targetComponent == minimal.MavComponent.ONBOARD_COMPUTER &&
packet.header.msgid === common.CommandLong.MSG_ID
){
Copy link
Owner

Choose a reason for hiding this comment

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

Space between brackets

@@ -100,6 +100,14 @@ class mavManager {

// send off initial messages
this.sendVersionRequest()

// Respond to MavLink commands that are targeted to the companion computer
Copy link
Owner

Choose a reason for hiding this comment

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

Remove any extraneous trailing whitespaces from this section

Addresses feature request stephendade#169.
Enables Rpanion to respond to send COMMAND_ACK and VIDEO_STREAM_INFORMATION messages via MAVLink in response to a MAV_CMD_REQUEST_MESSAGE.
Adds a React Select dropdown to the Video page to select which IP will be transmitted as part of the video stream URI that is sent in the MAVLink message.
@ddd999
Copy link
Author

ddd999 commented Mar 12, 2024

Thanks for the review. I fixed the whitespace issues in mavManager.js and confirmed it didn't give any warnings with npm run lint. Added your suggestion for the Video page layout also--appreciate that input as I wasn't sure how to integrate the new option.

@stephendade
Copy link
Owner

Looks good! Merging...

@stephendade stephendade merged commit d5f5820 into stephendade:master Mar 13, 2024
9 checks passed
@ddd999 ddd999 deleted the videostreaminfo branch March 13, 2024 01:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants