-
Notifications
You must be signed in to change notification settings - Fork 20
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
Adding a timestamp to debug messages #61
Adding a timestamp to debug messages #61
Conversation
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.
Thanks for the PR 👍
I've left a couple in-line comments.
Besides those: IIUC, this returns time in 'local time'. Many controllers will not be configured with the correct timezone (if that is even possible), and will likely not have NTP setup.
This means stamps prefixed to debug output would not necessarily show the correct time, when compared to the time of the receiving systems.
In a previous version I believe you used the time from the host, synced using the micro-ROS Agent. That would seem to avoid the timezone issue.
Did that not work correctly?
src/Debug.c
Outdated
@@ -36,6 +36,7 @@ void Ros_Debug_Init() | |||
void Ros_Debug_BroadcastMsg(char* fmt, ...) | |||
{ | |||
char str[MAX_DEBUG_MESSAGE_SIZE]; | |||
char Formatted_Time[300]; |
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.
Please use a named constant (ie: #define
) for the buffer size instead of a magic nr.
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 will make that change
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.
It doesn't appear that Formatted_Time
is used in the code below. Looks like a leftover artifact from debugging. I recommend removing this unused variable.
(If you do want to use it for something, be sure to replace the 300
with FORMATTED_TIME_SIZE
.)
gavanderhoorn is right, we have used another time function that is synchronized with the PC running the microROS Agent. motoros2/src/PositionMonitor.c Lines 404 to 406 in 9f1c599
|
I was looking at Yaskawa-Global:9f1c599...SejalBehere:7f163ac. That seems to do what you also suggest @ted-miller, but there must be a reason this didn't make it into this PR. Hence my question. |
In a previous version I did use time from the host, synced using the micro-ROS Agent. I was able to get the timestamp in the format of H:M:S.ms but was not able to add the day to it without adding the whole of what I have used now as well. I will make some more changes to the previous version and get the current timestamp format using the host time. |
…for debug messages
src/Debug.c
Outdated
builtin_interfaces__msg__Time debug_msg_timestamp; | ||
|
||
Ros_Nanos_To_Time_Msg(nanosecs, &debug_msg_timestamp); | ||
time(&debug_msg_timestamp); |
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.
IIUC, the time
function will put the current time into debug_msg_timestamp
. But, this is not needed because the current timestamp was already obtained above with rmw_uros_epoch_nanos
. I think that this line can be deleted.
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 did try by removing that line but what is happening is that for the fist few messages, before the message "Attempting to connect to the micro-ROS agent", the timestamp being showed is the epoch time (THU 1970-01-01 00:00:00.000) and it is only after this message that it shows the current timestamp.
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 see. Then I would suggest making the time source conditional on whether the agent is connected.
if (g_Ros_Communication_AgentIsConnected)
{
//get synchronized time from the agent
int64_t nanosecs = rmw_uros_epoch_nanos();
Ros_Nanos_To_Time_Msg(nanosecs, &debug_msg_timestamp);
}
else
{
//rmw_uros_epoch_nanos cannot sync with agent because it's not connected
time(&debug_msg_timestamp);
}
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.
Okay, I will replace the time function with the conditional
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 you can pass an instance of builtin_interfaces__msg__Time
to time(..)
.
@SejalBehere: if this is ready for another review please either request one or comment. |
@gavanderhoorn This is ready for another review. |
I have made the changes to address the points raised in the above review. Also, I have removed the +1 as like @ted-miller said str buffer is getting zeroed at the top and hence there's no need for considering the null terminator. |
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.
The only other question I have is to confirm whether you have a tested this on a live controller. If so, please post an example copy/paste of the output so we can see the final product.
Thanks!
src/Debug.c
Outdated
@@ -36,6 +36,7 @@ void Ros_Debug_Init() | |||
void Ros_Debug_BroadcastMsg(char* fmt, ...) | |||
{ | |||
char str[MAX_DEBUG_MESSAGE_SIZE]; | |||
char Formatted_Time[300]; |
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.
It doesn't appear that Formatted_Time
is used in the code below. Looks like a leftover artifact from debugging. I recommend removing this unused variable.
(If you do want to use it for something, be sure to replace the 300
with FORMATTED_TIME_SIZE
.)
Also, I owe you an updated version of |
I have made the changes to the branch. I did test this on a live controller and the output was obtained in the following format: [1687983037.99751401] [192.168.1.31:58649]: WED 2023-06-28 16:10:25.768 Using ROS domain ID: 0 |
Looks good to me. Thanks for the contribution! I'll work on resolving the conflicts this afternoon and get this merged. |
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.
See my comment.
914a805
to
ad5a349
Compare
Header updated with just the changes to localtime_r
Building this for DX200 I get several warnings, with one sticking out:
that would be this line. @SejalBehere: could you confirm the YRC build doesn't |
I can confirm that YRC does raise a warning. It's wanting a |
@gavanderhoorn, I'm also getting a warning for |
Not on DX200. See #18 (comment). |
PR Yaskawa-Global#61 added a timestamp to the log message sent out by MotoROS2. Use that timestamp instead of calculating a new one upon reception. The format of the timestamp prefixed by MotoROS2 is exactly the same as what `debug_listener.py` used to prefix. But due to limitations on the VxWorks side, will likely not take time-zones into account.
Changes were made to Debug.c to include a timestamp (Format - Day Y:M:D H:M:S.ms) in front of the debug messages being printed. Change was made to MotoROS_PlatformLib.h to add the function localtime_r. These changes were "Fixes #38" to resolve the issue "Add timestamp to debug udp broadcasts" .