-
Notifications
You must be signed in to change notification settings - Fork 37
Using message_encode_decode to send MAVlink messages #247
Using message_encode_decode to send MAVlink messages #247
Conversation
maxlou05
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.
Reviewed
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.
Where is this change? Can you make this a PR in common or something?
| cls, | ||
| home_position: position_global.PositionGlobal, | ||
| local_logger: logger.Logger, | ||
| worker_id: worker_enum.WorkerEnum, |
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.
Instead of having it as an input, the communications worker/class should always use the communication worker id. Just use the enum when you need to as a constant instead of as an input
| result = self.controller.send_statustext_msg(message) | ||
| if not result: | ||
| self.__logger.error("Failed to send statustext message", True) | ||
|
|
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.
Note that message may be None due to get_nowait, so you should not log error every time. Skip normally if its None
| input_queue: queue_proxy_wrapper.QueueProxyWrapper, | ||
| output_queue: queue_proxy_wrapper.QueueProxyWrapper, | ||
| communications_output_queue: queue_proxy_wrapper.QueueProxyWrapper, | ||
| coordinates_input_queue: queue_proxy_wrapper.QueueProxyWrapper, |
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.
This must be the second queue becasue that's what you defined in main_2024. It goes in order (input1, input2, output1, output2...)
| try: | ||
| coordinate = coordinates_input_queue.queue.get_nowait() | ||
| except queue.Empty: | ||
| local_logger.warning("No more coordinates to process") |
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.
No need to log this, as it will log it 8 times per second, and it's normal operation
| if not isinstance(coordinate, bytes): | ||
| local_logger.warning(f"Skipping unexpected input: {coordinate}") | ||
| continue |
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.
Do not put this check here, put it in the run() function. This is so that it still continues to put odometry in the output queues, and you don't skip that functionality of the flight interface worker (cuz now you'll just skip getting odometry every time you don't have a communications message)
| COMMUNICATIONS_WORKER_ID = worker_enum.WorkerEnum.COMMUNICATIONS_WORKER | ||
|
|
||
|
|
||
| def simulate_communications_worker( |
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.
No need to add this test, as Jane is doing it as her task. Just make sure that the old test can still run (ie fix the inputs to the functions and worker)
… function, adjusted flight interface test to reflect new inputs
maxlou05
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.
Reviewed. Conditionally approved after fix and common PR
| self.__logger.info(str(odometry_and_time_object), True) | ||
|
|
||
| if not isinstance(message, bytes): | ||
| self.__logger.warning(f"Skipping unexpected input: {message}") |
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.
Maybe don't log this? It will be kind of very spammy
maxlou05
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.
Reviewed
| result, metadata = metadata_encoding_decoding.encode_metadata( | ||
| worker_enum.WorkerEnum.COMMUNICATIONS_WORKER, len(list_of_messages) | ||
| ) | ||
| if not result: | ||
| local_logger.error("Failed to encode metadata", True) | ||
| continue |
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.
Could you move this thing to communications.py? Then this file will no longer need to import enum and encoding, and just do sending.
|
|
||
| self.__logger.info(str(odometry_and_time_object), True) | ||
|
|
||
| self.controller.send_statustext_msg(message) |
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.
Check if message is None. If it's None, do not send any message and just continue normal operation
| return self.__home_position | ||
|
|
||
| def run(self) -> "tuple[bool, odometry_and_time.OdometryAndTime | None]": | ||
| def run(self, message: bytes) -> "tuple[bool, odometry_and_time.OdometryAndTime | None]": |
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.
message: bytes | None
maxlou05
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.
Reviewed
| self.__logger.error("Failed to encode metadata", True) | ||
| return False, None, None | ||
|
|
||
| return True, encoded_position_global_objects, metadata |
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.
Just switch the order: metadata then encoded_position_global_objects. Make sure to change function definition and also communications worker.
maxlou05
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.
Approved
Fixed PR with cleaner commit history.
Description:
Used the following functions in data_encoding and flight_controller.py to send MAVlink messages to the flight controller via the flight interface worker:
encode_metadata()
encode_position_global()
send_statustext_msg()