-
Notifications
You must be signed in to change notification settings - Fork 3
Feature/exit state #241
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
base: develop
Are you sure you want to change the base?
Feature/exit state #241
Conversation
ewad3
left a comment
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.
Everything looks good. We just will need to keep an eye on the landing.
flight/utils/movement_controller.py
Outdated
| abs(x) <= reference_x * config.POINT_PERCENT_ACCURACY | ||
| and abs(y) <= reference_y * config.POINT_PERCENT_ACCURACY | ||
| ): | ||
| if abs(x) <= reference_x * 0.15 and abs(y) <= reference_y * 0.15: |
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 understand what you are doing here, but on the physical drone having to high of point accuracy causes the drone to "miss" its target and fly back and forth like it did at the flight test while trying to land.
pieperm
left a comment
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'm a bit hesitant to make changes to run.py before a flight test since it is so crucial. I think it might be a good idea to keep this on its own branch up through the flight test (similarly to how the unit test refactor will be kept on its own branch). We can run a test with this by switching to the branch during the flight test, and then if everything runs smoothly, we can merge it afterwards. What do you think about this approach?
run.py
Outdated
| flight_process: Process = init_flight(flight_args) | ||
| flight_process.start() | ||
| except KeyboardInterrupt: | ||
| except KeyboardInterrupt or AttributeError: |
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'm not sure if this is the right syntax for catching multiple errors. See https://stackoverflow.com/questions/6470428/catch-multiple-exceptions-in-one-line-except-block
Let me know if I'm wrong about this.
run.py
Outdated
| comm_obj.set_state("land") | ||
| flight_process: Process = init_flight(flight_args) | ||
| flight_process.start() | ||
| except TimeoutError or flight.DroneNotFoundError: |
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.
Same issue as mentioned before
|
I can wait to merge the pull request, the points Michael made are valid. I'm not too concerned with merging the request, I'd just like to make sure everything looks decent |
pieperm
left a comment
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.
Looks ready to test at the flight test! If all goes well, we can merge it after
…to feature/exit_state � Conflicts: � run.py
Addition of try-except block to handle Ctrl-C exit of flight and any errors that occur. Also added logging statements to keep track of errors.