[SW-957] make decorator for trigger services #578
Conversation
tcappellari-bdai
requested review from
METEORITENMAX,
jbarry-bdai,
amessing-bdai,
mpickett-bdai,
bhung-bdai and
khughes-bdai
as code owners
Pull Request Test Coverage Report for Build 13336683414Details
💛 - Coveralls |
tcappellari-bdai
requested review from
a team,
mbrauckmann-bdai,
jcarpinelli-bdai,
bjin-bdai,
mgoh-bdai,
mvictora-bdai and
amessing-bdai
| kwargs: Optional[dict | None] = None | ||
|
|
||
| def __init__( | ||
| self, service_func: Callable, service_name: str, *args: Optional[Any], **kwargs: Optional[Any | None] |
There was a problem hiding this comment.
| self, service_func: Callable, service_name: str, *args: Optional[Any], **kwargs: Optional[Any | None] | |
| self, service_func: Callable, service_name: str, *args: Optional[Any], **kwargs: Optional[Any] |
nit: does Any cover the None type?
| self.args = args | ||
| self.kwargs = kwargs or {} | ||
|
|
||
| def create_service(self, obj: Any, group: CallbackGroup) -> None: |
There was a problem hiding this comment.
obj: Any is pretty vague. Based on the naming as well it seems like this obj type is limited to a set of Spot objects?
There was a problem hiding this comment.
ya its supposed to be SpotRos object but that would cause a circular dependency. Was unsure best way around that and keep mypy happy
| obj.create_service(Trigger, self.service_name, self.callback, callback_group=group) | ||
|
|
||
| def callback(self, request: Trigger.Request, response: Trigger.Response) -> Trigger.Response: | ||
| if self.spot_ros.mock: |
There was a problem hiding this comment.
If this callback explcitly depends on SpotRos object, then does it make sense that TriggerServiceWrapper is its own class at all? Could the code duplication also be solved by moving these function to member functions of SpotRos? (and the member variables to this class can be stored as a dictionary, or wrapped in a function wrapper tool?)
The circular dependency comment suggests that there is a different way to implement this... and I'm thinking if this class specifically only works on SpotRos object, then they are functions/properties that belong to SpotRos class
There was a problem hiding this comment.
ya it makes sense to put it in spotROS. I just wanted to reduce the amount of code in it since its already m a s s i v e
tcappellari-bdai
force-pushed
the
SW-957-make-decorator-for-trigger-services-in-spot-ros-2
branch
from
fb2e7cb to
2c2b71c
Compare
tcappellari-bdai
deleted the
SW-957-make-decorator-for-trigger-services-in-spot-ros-2
branch
Change Overview
Made a descriptor class for trigger services to reduce boilerplate + copy/paste code in trigger service handlers
Testing Done
Tested all trigger services on robot. Found a bug with estop handling within
spot_wrapper(will make a new ticket to track this but out of scope for this PR)Please create a checklist of tests you plan to do and check off the ones that have been completed successfully. Ensure that ROS 2 tests use
domain_coordinatorto prevent port conflicts. Further guidance for testing can be found on the ros utilities wiki.