[jsk_topic_tools][connection_based_nodelet] warn if onInitPostProcess is not called

[furushchev]

Inspired from jsk-ros-pkg/jsk_recognition#2043 (comment)

[wkentaro]

Nice feature. Why we need the param no_warn_on_init_post_process?
Is it not good of warning always?

[furushchev]

@wkentaro I'd like to, but the current implementation does not monitor exact end of onInit execution, but only wait 5 seconds and check if it is done, so if we have some process that takes more than 5 seconds, it always warns if onInit is called. I mean the option is advanced one for that case.

[wkentaro]

Ok, so it depends on the implementation of the nodelet and there are few reasons why we use rosparam for that option, right?
Maybe you can set the duration of the timer as the data field.

[furushchev]

@wkentaro I think if we could mind about the duration, we already could notice that we forgot onInitPostProcess, so I don't think we want to change duration but just on / off.
If you prefer not to use rosparam, how about calling like: onInitProcess(/*warnOnInitPostProcess=*/ false) ? (None the less, I prefer rosparam because we can't know how long it takes on onInit beforehand.)

[wkentaro]

But if you want to change on/off, you already notice that you forget onInitPostProcess, right?
And if you know the duration is around 10s, in standard PC or Robots, you set duration as 10s. This is the reason why I suggest the param of duration.
I'm not so particular about the use of rosparam and configurable duration, but not sure why you need the flag ~no_warn_on_init_post_process.

[wkentaro]

[%s] should be set using ROSCONSOLE_FORMAT.
http://wiki.ros.org/rosconsole

export ROSCONSOLE_FORMAT='[${severity}] [${time}]: ${message}'

[wkentaro]

I'm not sure exactly why you need this.

[furushchev]

But if you want to change on/off, you already notice that you forget onInitPostProcess, right?

Right, but even if you remember and write onInitPostProcess at the end of onInit, this warning will be shown if onInit takes longer than 5 secs.
If reconfigurable parameter is confusing, I think it's also good idea to pass it like:

void HogeNodelet::onInit() {
  DiagnosticNodelet::onInit(/*warn onInitPostProcess = */ false);
  // PROCESS may take more than 5 seconds
  onInitPostProcess();
}

But I thought that non-advanced developers may copy & paste the first line, so I chose to use parameter.

[wkentaro]

Yeah. So I thought configurable duration was better.

But I thought that non-advanced developers may copy & paste the first line, so I chose to use parameter.
Add your review

I think non-advanced developers won't notice the rosparam, either.
And +1 for warn_on_init_post_process flag, and +10 for expected_duration_of_on_init and use it also for timer_warn_never_subscribed_ which also use 5s for the duration.

[furushchev]

@k-okada Any comment? If timedout I'll change as @wkentaro says.

[wkentaro]

?

[k-okada]

sorry, I did not catch the discussion,

warn_on_init_post_process flag: true
expected_duration_of_on_init : 5

and

+      NODELET_WARN("[%s] onInitPostProcess is not yet called. You are forgetting to call this in the end of onInit or onInit takes more than ~d seconds", nodelet::Nodelet::getName().c_str(), expected_duration_of_on_init  );

[wkentaro]

yeah, there may be some better param names like on_init_duration.