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

Missioncontrol addbasestationstates #44

Merged
merged 9 commits into from
Jan 4, 2025

Conversation

Quashnock
Copy link
Collaborator

Summary
Over the past couple days I tried working on this issue a bit on the new addbasestationstates branch. As it currently stands I have implemented the necessary UI and created a store for the base station operation states specified above, however I am not certain if the topic publishing works. I intend to add the spacebar hotkey disable in the future. I would appreciate feedback on the UI and implementation, particularly if the topic publishing works and if the newly added store is factored well.

UI
I chose to implement the select as a dropdown selector in the NavBar, due to the fact that only one can be selected at a time and I view the operation mode as an important piece of information to be able to view and switch on any screen, especially in emergency circumstances when the rover needs to be quickly disabled. Adding this extended the states section so that the page selection section could run out of space, so I set it so the page selection section grows to whatever length it has and can scroll horizontally for more space. I am considering a better way to communicate this visually.

Implementation
I chose to add a slice of the store for the operation state because it may be valuable for other components to know the current operation state, and with my current implementation the value of the operationState should be the single source of truth for what state the base station is currently in. However, I did run into an issue with importing the OperationState type from the store into the NavBar as I was receiving an error that the import could not recognize my type export. I am not too familar with TypeScript or Vue, so from what I was able to gather this may be an issue with a TSconfig we have. I have currently redefined the type in NavBar to make it function, and while I recognize this is not best practice it may not be too big of an issue to just keep around. Finally, I created a topic called "/setOperationState" that store publishes to everytime the state updates with a string containing the state to update to. I am not certain at this time if it functions as intended or if it works well with the backend robot code.

Thank you for reading and let me know if you have any questions or feedback!

Quashnock added 2 commits December 22, 2024 13:47
…perationStateStore.ts to store the current state of the base station.
… to the store and set up ROS topic publishing.
@Quashnock Quashnock linked an issue Dec 25, 2024 that may be closed by this pull request
Copy link
Collaborator

@adamseth2 adamseth2 left a comment

Choose a reason for hiding this comment

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

Confirmed it is publishing the message correctly!
image

The only thing I see is that the operation status is not distinct enough as its probably very important to see it at a glimpse on the status of the rover (hard to tell just by text, so color = good tldr). I suggest making it a whole different color like red for stop, purple for auto and green for tele mode.

The styles for option are good though! I would suggest moving the current option styles for the operationstate to become global scss to become what it should look by default and then modify this option more manually. If possible, it probably look best if it took up the whole width WIDTH of the navbar or most of it too.

src/components/Navbar.vue Outdated Show resolved Hide resolved
src/components/Navbar.vue Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@adamseth2
Copy link
Collaborator

Good work though! and I would not worry about the space bar as I believe @uellenberg is working on that for the controller/keyboard. But thanks for thinking ahead because yeah I think there potentially could be a problem with the UI state not matching up with the rover. I believe you can do this with option tags but make it so that the value of the option tag is binded by the operationState from the store (similar example in exampleComponent.vue for input tags). This would allow if the store value changes from like a keyboard/controller input, it would be as simple as changing the store value to update the navbar state too. Lmk if that doesn't make sense

@Quashnock
Copy link
Collaborator Author

Hi Adam and Jonah! I just made a commit that made all the changes and fixes you specified with your comments.

Additionally, I swapped the dropdown menu for a set of buttons since I couldn't find a way to get the dropdown menu to display the correct state if it is changed elsewhere. I also think this reads better and lets users swap modes quicker. The correct button should always be highlighted with the specified color in coordination with the operation state held in the store. This way you can use the state setter in other parts of mission control and the buttons should highlight accordingly.

Thanks for your pointers and let me know if you have any other questions for me!

@adamseth2
Copy link
Collaborator

Looks great! Love the button groups and agree they look nicer!

Just a couple of small things:

  1. Rather than "Off", name it "Disable" because I think it can be confusing what "off" means because Im 99% sure we just want the motors to completely turn off and not all other systems like GPI and cameras.

  2. Though github workflow says successfully passed the test (a little annoying that github does that), when you open it up, it shows 3 lint issues. Though small, fixing them will ensure the lint checker only gives warnings for new PRs
    image

  3. Run these commands in the terminal to reset the package .jsons back to the master branch because it doesn't seem like the changes are needed

git checkout origin/master -- package.json 
git checkout origin/master -- package-lock.json

Copy link
Collaborator

@uellenberg uellenberg left a comment

Choose a reason for hiding this comment

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

I want to make sure everything gets synchronized correctly, but we can do that at a later date. Right now, it looks like operation state is set to disabled by default, but that isn't synchronized by the rover. If we reload the page, the operation mode will say disabled, even though the rover might be in autonomous mode, which could be confusing.

For now though, great work, and thanks for putting up with all our nitpicks 😆

@adamseth2
Copy link
Collaborator

I want to make sure everything gets synchronized correctly, but we can do that at a later date. Right now, it looks like operation state is set to disabled by default, but that isn't synchronized by the rover. If we reload the page, the operation mode will say disabled, even though the rover might be in autonomous mode, which could be confusing.

For now though, great work, and thanks for putting up with all our nitpicks 😆

Yeah I realized when looking at the logic also that this is a fundamental problem overall with the mission control. There are a couple of ways to fix it, but I have to think about it more. Today or tomorrow I'll write a message of ideas to fix it, so we can figure what the best method. But yep for now, good work 🤘

background-color: hsl(300, 100%, 23%);
}

button#teleoperation-button.checked {
Copy link
Collaborator

Choose a reason for hiding this comment

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

My bad for not noticing this earlier, but let's use the power of scss to make this even cleaner.

Basically instead of repeating the word button, just use &

button {
        height: 80%;
        cursor: pointer;
        align-items: center;
        padding: 0 1rem;
        border-radius: 4px;
        background-color: transparent;
        color: white;
      }
      &:hover:not(.checked) {
      ...
      }
      &disabled-button.checked {
      ...
      }
}
...

Copy link
Collaborator

@adamseth2 adamseth2 left a comment

Choose a reason for hiding this comment

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

Thanks again for your hard work! Other than that extremely small thing, looks good and it is ready to merge

@Quashnock Quashnock merged commit 6a20151 into master Jan 4, 2025
2 checks passed
@Quashnock
Copy link
Collaborator Author

I've made the final changes and merged the branch with master. Since we still have the issue with the operation state desynchronizing with the rover when the page reloads I will keep the branch and GitHub issue open for now. Thanks for all of your help and feeback throughout this process, I'm glad we ran into some common issues now so that future work can go even smoother!

@adamseth2
Copy link
Collaborator

I've made the final changes and merged the branch with master. Since we still have the issue with the operation state desynchronizing with the rover when the page reloads I will keep the branch and GitHub issue open for now. Thanks for all of your help and feeback throughout this process, I'm glad we ran into some common issues now so that future work can go even smoother!

Congrats on the first PR! And yeah FYI the desynchronizing issue is not just with yours but also stuff like the arm mode, but glad we noticed it now

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.

Add Disabled/Teleoperation/Autonmous Buttons
3 participants