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

Change Altitude Mini-Widget #747

Merged

Conversation

ericjohnson97
Copy link
Contributor

@ericjohnson97 ericjohnson97 commented Feb 20, 2024

Adding the functionality to command altitude changes

closes #581

2024-02-19.21-32-08.mp4

Copy link
Member

@rafaellehmkuhl rafaellehmkuhl left a comment

Choose a reason for hiding this comment

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

Cool!

Curiosity: when you send a goTo message with X and Y being 0, MAVLink ignores those and just change the altitude?

src/libs/vehicle/ardupilot/ardupilot.ts Outdated Show resolved Hide resolved
@ericjohnson97
Copy link
Contributor Author

Cool!

Curiosity: when you send a goTo message with X and Y being 0, MAVLink ignores those and just change the altitude?

It must. I captured a change altitude command from QGC and copied it. Maybe I should test it with XY ignore bits set just incase the functionality ever changes.

@rafaellehmkuhl rafaellehmkuhl added the docs-needed Change needs to be documented label Feb 21, 2024
@ericjohnson97 ericjohnson97 force-pushed the feature/change_altitude branch from c468aa6 to f0172a8 Compare February 22, 2024 04:08
@ericjohnson97
Copy link
Contributor Author

Cool!
Curiosity: when you send a goTo message with X and Y being 0, MAVLink ignores those and just change the altitude?

It must. I captured a change altitude command from QGC and copied it. Maybe I should test it with XY ignore bits set just incase the functionality ever changes.

I tried with XY ignored and ardupilot did not respond for some reason. I don' have time tonight to fully investigate this, but given that QGC uses this exact format I think it should be safe.

@ericjohnson97
Copy link
Contributor Author

@rafaellehmkuhl I see the needs documentation tag is set, does that content need to be updated here?
https://github.com/bluerobotics/ardusub-zola

what are the expectations?

@ES-Alexander
Copy link
Contributor

ES-Alexander commented Feb 22, 2024

what are the expectations?

We haven't yet written contributing guidelines there unfortunately (sorry!).

New contributions to Cockpit should (almost always) be documented with PRs to the Cockpit-devel-temp branch.

For this particular feature I'd recommend adding a bullet point for the new widget in the mini widgets section (of the advanced-usage docs) describing what it does / what it's for, and include some numbered bullets within that describing the process of using it (since it's not just a single button press), preferably with a screenshot showing the relevant UI elements (e.g. if you screenshot a display similar to 0:05 from your current video, you could add some numbers to the image showing which elements are interacted with at each part of the process).

As a few notes:

  • For the numbering it's preferred to use repeated 1.s, because the markdown renderer should auto-number them from there, and it reduces changes if the steps get modified in future, e.g.
    1. Click the `Change Alt` mini-widget button
    1. Set the new altitude target on the vertical slider that appears on the right side of the screen
    1. Slide to confirm
    
  • For screenshots that cover more than a single UI element, it can be nice if they're taken on a reasonably small window, so that the elements of interest appear large (e.g. you can shrink your browser window a bit)
  • It's convenient if documentation PRs include links to the relevant functionality PRs they're documenting
    • e.g. you can write documents bluerobotics/cockpit#747 in the PR description
  • If you're wanting to build the docs to test the output we also don't yet have decent instructions for that, but basically you'll need to
    1. install Zola
    2. Clone the repo on the main branch
    3. Initialise submodules
    4. Replace the contents of the relevant submodule folder with your changes folder
    5. Build the docs (using zola serve)
    6. Access to the generated http server from your browser, and check the docs at the relevant location
    7. Iterate
    8. PR when ready
      • you're welcome to open a draft PR if you want progress to be available before you specifically want it reviewed

An example command line log might look like

# clone your branch from your fork (should be based off Cockpit-devel-temp from the bluerobotics upstream)
git clone https://[email protected]/MY_USERNAME/ardusub-zola -b My-Change-Branch-Name cockpit-docs-working
cd cockpit-docs-working # enter your changes folder
... # make your code changes
cd .. # go back to where you were (I normally just have two terminal tabs open, for easier switching)
# clone the main docs repo for building (can do from your fork, but not necessary just for building)
git clone https://github.com/bluerobotics/ardusub-zola ardusub-zola-building
cd ardusub-zola-building # enter your building folder
git submodule update --init --recursive # get the submodules
rm -r 'content/software/control-station/Cockpit - 1.0' # delete the current docs (note different structure if using BlueOS-deploy branch instead of main)
cp -r '../cockpit-docs-working' 'content/software/control-station/Cockpit - 1.0' # copy in your modified docs
zola serve # this needs to be done here, from the root of the build directory (e.g. not from in a content folder)
... # go to the generated http server, and navigate to /software/control-station/Cockpit - 1.0/advanced-usage
... # iterate
... # push your changes to your remote, and PR to the bluerobotics Cockpit-devel-temp branch when ready

Copy link
Member

@rafaellehmkuhl rafaellehmkuhl left a comment

Choose a reason for hiding this comment

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

Looks good to me.

About the docs, since you've added several PRs that add functionality that don't have docs yet, I think we can merge this and you can work on the documentation for the other PRs all together.

@ES-Alexander what do you think?

@ES-Alexander
Copy link
Contributor

About the docs, since you've added several PRs that add functionality that don't have docs yet, I think we can merge this and you can work on the documentation for the other PRs all together.

@ES-Alexander what do you think?

That’s fine with me, just good if we can sort out the docs before stable, and if one docs PR is documenting several feature PRs then it’s good if it can still link to all of them :-)

@patrickelectric patrickelectric merged commit 275764b into bluerobotics:master Feb 22, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-needed Change needs to be documented
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change Altitude Widget
4 participants