Changing all ROS_RATE occurences to ROSParam #99

pranaypratyush · 2016-01-12T11:42:48Z

No description provided.

prudhvid · 2016-01-12T11:46:26Z

Great work! @pranaypratyush changing params over so many files is really appreciable 👍
cc @kalyan-kumar @ghostwriternr

pranaypratyush · 2016-01-12T11:50:55Z

@icyflame umm. ok but changing all those files again would take some time.
@prudhvid thanks.

icyflame · 2016-01-12T12:32:37Z

@pranaypratyush Use git rebase -i. Find a tutorial online, or get help from @kalyan-kumar. You will be able to change only the commit messages, and leave everything else untouched.

BTW, Good work 👍

icyflame · 2016-01-12T12:45:08Z

@pranaypratyush TODO:

We are not using Scripts/launch/*.launch files, only the Scripts/launch/simulator/*.launch and Scripts/launch/vehicle/*.launch. So, revoke your changes to those files. They are there for legacy reasons only, and may be removed with a commit down the line as well. (cc @nevinvalsaraj )
Change the handling of the case when the rosparam is not defined. Instead of setting a default rate, throw a RuntimeError and exit out of the node.

pranaypratyush · 2016-01-12T13:20:56Z

Shouldn't I use ROS_ERROR instead of RuntimeError ? And ros::shutdown() to shut the node ?

prudhvid · 2016-01-12T14:13:58Z

Yeah @pranay you should do that
On Jan 12, 2016 6:51 PM, "Pranay Pratyush" [email protected] wrote:

Shouldn't I use ROS_ERROR instead of RuntimeError ? And ros::shutdown()
to shut the node ?

—
Reply to this email directly or view it on GitHub
#99 (comment).

icyflame · 2016-01-12T16:54:02Z

@pranaypratyush @prudhvid ROS_ERROR is for printing, it's one of the log levels (like DEBUG, INFO, etc), not an exception.

In python the exception itself is called RuntimeError, so you should print to ROS_ERROR and then, throw a RuntimeError so that the stackTrace is available to the user.

pranaypratyush · 2016-01-12T17:59:07Z

@icyflame What about in c++ ? C++ doesn't show stack traces ( I think ). So do I use RuntimeError there too ?

icyflame · 2016-01-12T18:09:56Z

I think there's a mechanism. RuntimeError does not exist in C++ AFAIK. It's called something else. @kalyan-kumar ?

icyflame · 2016-01-12T18:11:20Z

btw @pranaypratyush Next time around google before coming here!

pranaypratyush · 2016-01-12T18:49:24Z

I actually google before asking here. Why the hell do you think there is so much time gap between the comments ? I found out that 'std::runtime_error is a more specialized class, descending from std::exception, intended to be thrown in case of various runtime errors.' But it won't show the stack trace (obviously). To show the stack trace I would have to do this.
Now the question is do I really need to go through all this ( I mean displaying an error saying 'parameter not present' seems pretty damn sufficient to me ).

icyflame · 2016-01-13T05:52:43Z

Um, so apparently, the stack trace can be seen by using gdb. So, you can peace-out on the stack trace, but the exception must be thrown, and the name of the parameter must obviously be a part of the message shown.

pranaypratyush · 2016-01-13T14:07:27Z

@icyflame @prudhvid made some changes take a look at them.

- Modified all occurences of ROS_RATE in .cpp files to accept a ROSParam double instead of always using a default value. - Fixes auviitkgp#98

- Modified all occurences of ROS_RATE in .py files to accept a ROSParam double instead of always using a default value. - 'yaml' file is yet to be added. - Fixes auviitkgp#98, only for `.py` files

- Created a new `yaml` file under `Scripts/launch/yaml/ros_rate_param.yaml`. - Modified all `.launch` files to load the `.yaml` file. - Fixes auviitkgp#98, adds `yaml` file

- Modifies `.py` files to throw `RuntimeError` and show stack trace when `/ros_rate` not found. - Modifies `.cpp` files to throw `runtime_error` when `/ros_rate` not found. - Reverts back legacy `.launch` files to original state. - Other fixes regarding `ros::NodeHandle`. - Fixes auviitkgp#98, exception handling and debug commit

icyflame · 2016-01-14T07:09:49Z

We are not using Scripts/launch/.launch files, only the Scripts/launch/simulator/.launch and Scripts/launch/vehicle/*.launch. So, revoke your changes to those files. They are there for legacy reasons only, and may be removed with a commit down the line as well. (cc @nevinvalsaraj )

@pranaypratyush

icyflame · 2016-01-14T07:14:15Z

sensor_stack/seabotix/src/testThrusters.py

-	else:
-	    rate = default_rate
-
+	    try:


try and except are not at the same indentation level.

icyflame · 2016-01-14T07:17:00Z

@pranaypratyush See the above line comments, and fix them. Also, revoke the changes done to legacy / unused files using git checkout indigo-devel <filepath>.

This will make the diff more streamlined.

icyflame · 2016-01-14T07:18:06Z

sensor_stack/nqDVL/src/nqDVL.py

+		sys.exit(0)
+
+	r = rospy.Rate(temp_rate)
+


@aniket11bh @kshitijgoel007 @VishnuDuttSharma We should change these rates to the global constant right?

aniket11bh · 2016-01-14T13:42:12Z

No, you should not change these rates. Because It is the frequency at which data is sent by the sensors. If the rate become low then it may cause buffer overflow causing blocking of data.

But, we can change the rate at which sensor data is published by the node. Which is not written separately in our current code.

Also, need to fix the problem of DVL package not getting any data somtimes.

icyflame · 2016-01-14T14:32:27Z

@pranaypratyush there you go. Revoke the changes in the sparton, nqDvl packages as well. Use git checkout indigo-devel)

pranaypratyush · 2016-01-14T15:06:06Z

So do I edit my last commit or create a new commit ?

icyflame · 2016-01-14T15:07:58Z

New commit.

- Also debugs some indentation issues that crept in during the last commit.

icyflame · 2016-01-18T15:38:35Z

@pranaypratyush Still to revert trax, sparton, nqDvl. Also, run the hooks/formatAll.sh script to make sure your CPP files are indented properly.

pranaypratyush · 2016-01-23T04:57:33Z

So I am reverting all changes to all files under sensor_stack. And why is there a branch conflict ? How do I know where it is ?

icyflame · 2016-01-23T05:05:40Z

@pranaypratyush Rebase this branch. Since this PR was taking too long, another PR ( #101 ) was merged into the main branch.

That moves all the unused files into obsolete/. So, when you are done with this PR, none of the files in obsolete should be touched.

pranaypratyush · 2016-01-23T05:40:55Z

@icyflame Ok so I have reverted all files in sensor_stack to the indigo-devel branch. And merged my indigo-devel branch to current. Everything seems okay now.

icyflame · 2016-01-23T05:55:48Z

@pranaypratyush LGTM. We will merge this once we have tested this on the vehicle.

icyflame · 2016-05-11T13:23:56Z

@pranaypratyush If you are testing the vehicle, you must test this out. And if it works, then we should immediately merge it. Not having it on master could cause problems later.

icyflame · 2016-08-22T12:16:15Z

@pranaypratyush @Nightfury18 This has been tested I think. The diff looks fine to me, and I will merge it by tonight unless one of you can bring up an issue with the PR :)

pranaypratyush force-pushed the ros_param branch from 3a12fc7 to ebe0dbb Compare January 12, 2016 16:00

pranaypratyush force-pushed the ros_param branch 3 times, most recently from 7be450c to 9e824d6 Compare January 13, 2016 13:41

icyflame changed the title ~~Fix for #98~~ Changing all ROS_RATE occurences to ROSParam Jan 13, 2016

pranaypratyush added 4 commits January 14, 2016 11:00

Enables '.cpp' files to use '/ros_rate' ROSParam

296e47d

- Modified all occurences of ROS_RATE in .cpp files to accept a ROSParam double instead of always using a default value. - Fixes auviitkgp#98

Enables '.py' files to use '/ros_rate' ROSParam

4ecd16c

- Modified all occurences of ROS_RATE in .py files to accept a ROSParam double instead of always using a default value. - 'yaml' file is yet to be added. - Fixes auviitkgp#98, only for `.py` files

Adds 'yaml' file to initialize '/ros_rate' ROSParam

f737821

- Created a new `yaml` file under `Scripts/launch/yaml/ros_rate_param.yaml`. - Modified all `.launch` files to load the `.yaml` file. - Fixes auviitkgp#98, adds `yaml` file

pranaypratyush force-pushed the ros_param branch from 9e824d6 to 1d8ed92 Compare January 14, 2016 05:37

icyflame reviewed Jan 14, 2016
View reviewed changes

sensor_stack/seabotix/src/testThrusters.py

else:

rate = default_rate

try:

Copy link

Member

icyflame Jan 14, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

try and except are not at the same indentation level.

icyflame reviewed Jan 14, 2016
View reviewed changes

Reverts legacy '.launch' files

9dde2c2

- Also debugs some indentation issues that crept in during the last commit.

Reverts all files in 'sensor_stack'

a826af0

icyflame added the can-merge label Jan 23, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changing all ROS_RATE occurences to ROSParam #99

Changing all ROS_RATE occurences to ROSParam #99

pranaypratyush commented Jan 12, 2016

prudhvid commented Jan 12, 2016

pranaypratyush commented Jan 12, 2016

icyflame commented Jan 12, 2016

icyflame commented Jan 12, 2016

pranaypratyush commented Jan 12, 2016

prudhvid commented Jan 12, 2016

icyflame commented Jan 12, 2016

pranaypratyush commented Jan 12, 2016

icyflame commented Jan 12, 2016

icyflame commented Jan 12, 2016

pranaypratyush commented Jan 12, 2016

icyflame commented Jan 13, 2016

pranaypratyush commented Jan 13, 2016

icyflame commented Jan 14, 2016

icyflame Jan 14, 2016

icyflame commented Jan 14, 2016

icyflame Jan 14, 2016

aniket11bh commented Jan 14, 2016

icyflame commented Jan 14, 2016

pranaypratyush commented Jan 14, 2016

icyflame commented Jan 14, 2016

icyflame commented Jan 18, 2016

pranaypratyush commented Jan 23, 2016

icyflame commented Jan 23, 2016

pranaypratyush commented Jan 23, 2016

icyflame commented Jan 23, 2016

icyflame commented May 11, 2016

icyflame commented Aug 22, 2016

Changing all ROS_RATE occurences to ROSParam #99

Are you sure you want to change the base?

Changing all ROS_RATE occurences to ROSParam #99

Conversation

pranaypratyush commented Jan 12, 2016

prudhvid commented Jan 12, 2016

pranaypratyush commented Jan 12, 2016

icyflame commented Jan 12, 2016

icyflame commented Jan 12, 2016

pranaypratyush commented Jan 12, 2016

prudhvid commented Jan 12, 2016

icyflame commented Jan 12, 2016

pranaypratyush commented Jan 12, 2016

icyflame commented Jan 12, 2016

icyflame commented Jan 12, 2016

pranaypratyush commented Jan 12, 2016

icyflame commented Jan 13, 2016

pranaypratyush commented Jan 13, 2016

icyflame commented Jan 14, 2016

icyflame Jan 14, 2016

Choose a reason for hiding this comment

icyflame commented Jan 14, 2016

icyflame Jan 14, 2016

Choose a reason for hiding this comment

aniket11bh commented Jan 14, 2016

icyflame commented Jan 14, 2016

pranaypratyush commented Jan 14, 2016

icyflame commented Jan 14, 2016

icyflame commented Jan 18, 2016

pranaypratyush commented Jan 23, 2016

icyflame commented Jan 23, 2016

pranaypratyush commented Jan 23, 2016

icyflame commented Jan 23, 2016

icyflame commented May 11, 2016

icyflame commented Aug 22, 2016