Conversation
sarthakdas
requested review from
cmower and
Copilot
and removed request for
Copilot
cmower
left a comment
cmower
left a comment
There was a problem hiding this comment.
Please address comment, and also include docstrings.
| def start_logging(self, channel: str, msg: string_t) -> flag_t: | ||
| log.info("Starting logging to file: " + msg.data) | ||
| self.proc = subprocess.Popen( | ||
| ['lcm-logger', msg.data], | ||
| stdout=subprocess.PIPE, | ||
| stderr=subprocess.PIPE, | ||
| start_new_session=True, | ||
| ) | ||
| msg = flag_t() | ||
| msg.flag = True | ||
| return msg | ||
|
|
||
| def stop_logging(self, channel: str, msg: flag_t) -> flag_t: | ||
| log.info("Stopping logging") | ||
| self.proc.kill() | ||
|
|
||
| msg = flag_t() | ||
| msg.flag = True | ||
| return msg | ||
|
|
There was a problem hiding this comment.
user may make a mistake and call "logger/stop" first, or "lower/start" twice
the request also no need to be a string_t, i think can just be empty bytes
the response should contain
bool: success # logger successfully started/stopped
str: message # when success=True, then something like "successfully started logger", if False then contain an informative message
maybe need a new type
There was a problem hiding this comment.
"the request also no need to be a string_t, i think can just be empty bytes" I think we should keep this to give users flexibility of the lcm-logger settings rather then only having default values.
There was a problem hiding this comment.
ok, then this line needs to be updated
log.info("Starting logging to file: " + msg.data)
cmower
left a comment
cmower
left a comment
There was a problem hiding this comment.
please address comments and also changes in Robotics-Ark/ark_types#15
| return out | ||
|
|
||
| try: | ||
| log.info("Stopping logging") |
There was a problem hiding this comment.
"stopped logging" also this line should appear after you called proc.kill()
| log.info("Stopping logging") | ||
| self.proc.kill() | ||
| self.proc.wait(timeout=5) | ||
| self.proc = None |
There was a problem hiding this comment.
dont we need to call del self.proc to make sure the memory is cleared?
There was a problem hiding this comment.
im not sure couldn't see a difference with/without including for safety?
| class LoggerNode(BaseNode): | ||
|
|
||
| def __init__(self, name: str, config: Optional[dict[str, Any]] = None): | ||
| r""" |
cmower
left a comment
cmower
left a comment
There was a problem hiding this comment.
@sarthakdas please address all comments, some were missed from before.
| return out | ||
|
|
||
| try: | ||
| log.info("Starting logging") |
| try: | ||
|
|
||
| self.proc.kill() |
|
|
||
| self.proc.kill() | ||
| self.proc.wait(timeout=5) | ||
| log.info("Stopping logging") |
Wrapping LCM logger in Services
2 participants
Wrapped LCM logger into a node and a service that takes in a file path