Roslibjs logging 211 #299

joshua-glazer · 2020-02-11T05:27:26Z

Assignee Section

hi it me

Description

Created js functions for logging to mimic rospy functions: logInfo, logWarn, logErr, logFatal
These all call rosLog which is assisted by rosTimeStamp and publishRosLog. A new publisher was created so that these functions all log to /rosout. In practice this means you'll find the logs in rosout.log on the computer which is running as ROS MASTER. The functions include the timestamp and the log level.

I've left some todos. Most of them are bonus features that aren't necessarily required. However one missing aspect is the logic for activating/deactivating logDebug as this would need some discussion on where we put the variable that handles this.

How to Test:

run the GUI with rosgui and startgui
open a terminal and subscribe to /rosout: rostopic echo /rosout
open the chrome console on the GUI and call the following functions one by one, passing in a string of your choosing as the parameter: logDebug, logInfo, logWarn, logErr, logFatal
verify that messages with the format [<loglevel>] [<timestamp>]: <message> are shown:
- in the chrome console, where debug/info are normal messages, warn is a warning, error and fatal are errors
- in the GUI console, where they all show up simply as text
- in the terminal, where they are shown with all of the parameters (seq, secs, nsecs, etc.)
- at the end of ~/.ros/log/latest/rosout.log. May require that you close roscore (and possibly all the other ros scripts) before the logfile is updated

Future potential issues:

determine how to generate a log file dedicated to the gui, since right now logs simply go to rosout.log whereas each python script (ex. ArmNode.py) gets its own log file (to be addressed in roslibjs logging functions log to rosout.log instead of dedicated logfiles #330)
consider implementing the *args and **kwargs style functionality of the rospy functions (to be addressed in Investigate need for optional arguments to roslibjs logging functions #331)
have a variable/parameter somewhere to turn off DEBUG log level messages, in initRosWeb() for example. For reference, in rospy it's done with rospy.init_node(nodeName, log_level=rospy.DEBUG) (addressed in Arm motor stutters when the page has been loaded for a while #321)
for multiple operators, have some way of distinguishing logs generated from each specific operator's laptop/gui. Not sure where in the log message this would go... I've seen sometimes logs will have a "[Client 0]" show up in the messages, maybe it's related. (to be addressed in roslibjs logging functions log to rosout.log instead of dedicated logfiles #330 or future issue)

closes #211

Approval from at least one software team lead is necessary before merging.

Reviewer Section

Aside from local testing and the General Integration Test it is implied that static analysis should be included in the verification process.

Local Test Performed Successfully
General Integration Test Performed Successfully

For Pull Requests that do not include code changes, it is not required to perform the tests above.

MartensCedric

Very hot @vashmata . Honestly I'm very impressed by this PR. It looks clean

robot/basestation/static/js/roslibjs/ros-helpers.js

joshua-glazer · 2020-02-12T05:29:22Z

I've made some significant refactorings to hopefully clean the code up more

robot/basestation/static/js/roslibjs/ros-helpers.js

Co-authored-by: MartensCedric <cedricmartens98@gmail.com>

MartensCedric · 2020-02-13T22:31:28Z

@PeterGhimself I approved

PeterGhimself

You need to include a step by step instruction on how to actually test your code. Just add it to your PR description and I'll test it right after.

PeterGhimself · 2020-02-14T20:05:53Z

robot/basestation/static/js/roslibjs/ros-helpers.js

@@ -188,6 +204,76 @@ function initRosWeb () {
 }

 /* functions used in main code */
+
+function rosTimestamp () {


When adding any new function good practice is to add a docstring for it.
Please add a high level overview of what each function does, in a sentence or two.

this should be resolved

@PeterGhimself It looks like I did add a docstring. Did I do it right?

PeterGhimself · 2020-02-14T20:15:34Z

robot/basestation/static/js/roslibjs/ros-helpers.js

+  let currentTime = new Date()
+  let secs = currentTime.getTime()/1000 // seconds before truncating the decimal
+  let secsFloored = Math.floor(secs) // seconds after truncating
+  let nsecs = Math.round(1000000000*(secs-secsFloored)) // nanoseconds since the previous second


I say rename this nanoSecs to make the name clearer

PeterGhimself · 2020-02-14T20:21:13Z

robot/basestation/static/js/roslibjs/ros-helpers.js

+  logData = {}
+  logData[ROSDEBUG] = {prefix : '[DEBUG]', isError : false}
+  logData[ROSINFO] = {prefix : '[INFO]', isError : false}
+  logData[ROSWARN] = {prefix : '[WARN]', isError : true}


lets do two things:

since semantically (and like in C++) warnings != erros this isError should be false

we should use console.warn('something')

PeterGhimself · 2020-02-14T20:24:45Z

robot/basestation/static/js/roslibjs/ros-helpers.js

+  stamps = rosTimestamp()
+  consoleMsg = logData[logLevel].prefix + ' [' + stamps[1] + ']: ' + message
+
+  !logData[logLevel].isError


Simpler is better. Either make if else statement or write a comment (in English) explaining what's going on plz

MartensCedric

Don't forget to open the issues of what you wrote on the PR if you want them to get implemented

PeterGhimself

Tested and everything seems good. Messages appear in console, in GUI console, as well as appearing in the ~/.ros/log/latest/rosout.log on a ROS master computer.

Great work 😍 😎

joshua-glazer added 5 commits February 10, 2020 17:25

[#211] Add js logging functions

c5d7c29

[#211] Add timestamps to log messages

eae6172

[#211] Fix typo

3e7b2c5

[#211] Refactor timestamp calculations

31bc68b

[#211] Replace time in seconds with hh:mm:ss format for gui console

d2e920d

joshua-glazer requested review from MartensCedric and PeterGhimself February 11, 2020 05:27

joshua-glazer assigned joshua-glazer and unassigned joshua-glazer Feb 11, 2020

MartensCedric assigned joshua-glazer Feb 11, 2020

MartensCedric requested changes Feb 11, 2020

View reviewed changes

robot/basestation/static/js/roslibjs/ros-helpers.js Outdated Show resolved Hide resolved

robot/basestation/static/js/roslibjs/ros-helpers.js Outdated Show resolved Hide resolved

joshua-glazer added 3 commits February 11, 2020 18:58

[#211] Adjust comments, remove todos, rename logGeneric

81adb32

[#211] Minor refactor timestamp function

9c46742

[#211] Refactor code to make it cleaner

3a985b8

joshua-glazer requested a review from MartensCedric February 12, 2020 05:29

MartensCedric requested changes Feb 13, 2020

View reviewed changes

robot/basestation/static/js/roslibjs/ros-helpers.js Outdated Show resolved Hide resolved

robot/basestation/static/js/roslibjs/ros-helpers.js Outdated Show resolved Hide resolved

robot/basestation/static/js/roslibjs/ros-helpers.js Outdated Show resolved Hide resolved

joshua-glazer and others added 2 commits February 13, 2020 17:05

[#211] Fix recommended changes by smartens

7517562

Co-authored-by: MartensCedric <cedricmartens98@gmail.com>

[#211] Remove useless console.log

0ab21c3

MartensCedric previously approved these changes Feb 13, 2020

View reviewed changes

PeterGhimself requested changes Feb 14, 2020

View reviewed changes

[#211] Adjust code based on Peter's request

57ade01

joshua-glazer dismissed MartensCedric’s stale review via 57ade01 February 15, 2020 03:14

joshua-glazer requested a review from PeterGhimself February 15, 2020 03:16

MartensCedric approved these changes Feb 15, 2020

View reviewed changes

PeterGhimself approved these changes Feb 15, 2020

View reviewed changes

PeterGhimself merged commit 0e37849 into master Feb 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Roslibjs logging 211 #299

Roslibjs logging 211 #299

joshua-glazer commented Feb 11, 2020 •

edited

Loading

MartensCedric left a comment

joshua-glazer commented Feb 12, 2020

MartensCedric commented Feb 13, 2020

PeterGhimself left a comment

PeterGhimself Feb 14, 2020

MartensCedric Feb 15, 2020 •

edited

Loading

joshua-glazer Jun 13, 2021

PeterGhimself Feb 14, 2020

joshua-glazer Feb 14, 2020

PeterGhimself Feb 14, 2020

joshua-glazer Feb 14, 2020

PeterGhimself Feb 14, 2020

joshua-glazer Feb 14, 2020

MartensCedric left a comment

PeterGhimself left a comment

Roslibjs logging 211 #299

Roslibjs logging 211 #299

Conversation

joshua-glazer commented Feb 11, 2020 • edited Loading

Assignee Section

Description

How to Test:

Future potential issues:

Reviewer Section

MartensCedric left a comment

Choose a reason for hiding this comment

joshua-glazer commented Feb 12, 2020

MartensCedric commented Feb 13, 2020

PeterGhimself left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartensCedric Feb 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartensCedric left a comment

Choose a reason for hiding this comment

PeterGhimself left a comment

Choose a reason for hiding this comment

joshua-glazer commented Feb 11, 2020 •

edited

Loading

MartensCedric Feb 15, 2020 •

edited

Loading