Conversation
cst0
left a comment
cst0
left a comment
There was a problem hiding this comment.
Super nice to have a gui, I think what you're working on here will be really helpful. But there's some cleanliness and message choices that might be worth taking another look at. It might also be worth looking at PFD's for inspiration of how this sort of data can be presented in a way that both provides the raw, verbose data, while also keeping that layout quickly visually accessible.
|
|
||
|
|
||
| if __name__ == '__main__': | ||
| listener() |
There was a problem hiding this comment.
it's not clear to me what happened with this file so I don't know why it's showing up in this commit, but I do recall regretting the fact that it was written (not by me). I'm guessing it's just a file rename?
| self.publisher.publish(ret_msg) | ||
|
|
||
| if __name__ == '__main__': | ||
| vehicle_stats() |
There was a problem hiding this comment.
this node just needed a bit of tweaking since there was a missing paren. Also note that 'Duration' doesn't operate in Hz, so if you want this called at 4 hz you have to give it a duration of 1/4. Also, although the timer takes in a parameter, it's there to denote event information (expected duration vs. actual duration). Finally, there were 3 different things called 'publisher', so I just removed the unused import and renamed the class variable.
| from PySide2.QtWidgets import QApplication, QWidget | ||
| from PySide2.QtGui import * | ||
| from PySide2.QtWidgets import * | ||
| from PySide2.QtCore import * |
There was a problem hiding this comment.
better practice is to use singular imports
| import sys | ||
| import rospy | ||
| import math | ||
| from PySide2extn.RoundProgressBar import roundProgressBar |
There was a problem hiding this comment.
and also good practice would be to sort these a tad
| fullscreenMode = True | ||
| # Enter Screen Resolution for surface station(Only if fullscreenMode is disabled) | ||
| height = 1080 | ||
| width = 1920 |
There was a problem hiding this comment.
global scoped constant variables that are lowercase (instead of conventional ALL_CAPS)? Alternatively, since you know the goal is for this to be full-screen, you could just collect those resolution values and go from there.
|
|
||
| # Code to be executed | ||
| if __name__ == "__main__": | ||
| import sys |
There was a problem hiding this comment.
why is this import here and not up at the top? Also, you may need to make use of rospy.myargv if you plan on running this as a node.
| sys.exit(app.exec_()) | ||
|
|
||
|
|
||
| window() No newline at end of file |
There was a problem hiding this comment.
feels like there's a fair chunk of duplicated code between this and the previous node, why is that?
|
oh, also, it feels like a lot of this PR is about moving things around, which is pretty odd for a PR titled 'GUI'... |
* Fixed mag orientation
* Added bar30 and ping360 nodes * Added sonar code * updated launchfile for bar30 * Updated with dos2unix * added param for fluid den * Fixed temp/depth reversal Co-authored-by: Hank <hkmpham147@gmail.com>
* Added PID * Added dynamic reconfig * Repaired PID on vehicle * Updated duty cycle values in launch file for vehicle * added time delay in PID for init cycle * Updated setpoint functon * Rearranged launch files * Fixed pitch and yaw sources * Repaired PID * Updated msgs Co-authored-by: HkPPP <hkmpham147@gmail.com> Co-authored-by: Hank Pham <46695607+HkPPP@users.noreply.github.com>
* Added accel offset
3 participants
No description provided.