-
Notifications
You must be signed in to change notification settings - Fork 2
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
Conversation
…perationStateStore.ts to store the current state of the base station.
… to the store and set up ROS topic publishing.
There was a problem hiding this 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!
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.
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 |
…s and made general bug fixes.
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! |
…mall requested changes.
There was a problem hiding this 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 😆
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 🤘 |
src/components/Navbar.vue
Outdated
background-color: hsl(300, 100%, 23%); | ||
} | ||
|
||
button#teleoperation-button.checked { |
There was a problem hiding this comment.
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 {
...
}
}
...
There was a problem hiding this 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
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 |
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!