Slightly improved architecture of robot.yaml #177

JanezCim · 2021-09-09T13:22:24Z

Changes in how core.launch gets generated and launched. Biggest change: base.yaml does not exsist any more, everything gets loaded through magni_bringup/config/default_robot.yaml -> launch_core.py -> generated_core.launch. This way the params from base.yaml are not duplicated and hopefully makes the whole process more clear.

Fixes #172
Fixes #169

on the rpi that runs noetic test with
python3 ~/catkin_ws/src/magni_robot/magni_bringup/scripts/launch_core.py --debug

when reviewing:

go over the code to see if there are any stupid mistakes, typos
play with /etc/ubiquity/robot.yaml
add new test extrinsics to as ~/.ros/extrinsics/extrinsics.yaml
also do python3 ~/catkin_ws/src/magni_robot/magni_bringup/scripts/launch_core.py which actually launches the generated_core.launch to see if everythig starts up as expected - try to move the robot around and stuff.
in yaml files do a typo test to see if the launch_core.py handels it.
try to break this in other ways.

rohbotics · 2021-09-20T18:57:43Z

Hey @JanezCim Can you separate out the param changes from the other changes here? I think the flat parameters need more discussion, but the rest of this is pretty good.

JanezCim · 2021-09-21T08:16:12Z

Hmm ok, I think it would be best that we continue discussion about the flat parameters here and I make a separate PR fixing the install rules.

Why I made the flat params:

its way easier to make per-param replacement with flat params (you dont have to loop within the sub dictionaries that non-flat params had)
it makes the params more logical (especially true for lidar and camera locations) (why have raspicam: {'camera_installed' : 'True', 'position' : 'forward'} when you can simply have raspicam_position: 'forward' - the camera installed is a bit redundant i think
Since this is noetic, we dont have to worry about backwards compatibility

Bad things:

old params will not work here (instead the script will do warning and launch with default params) - do we need that robot.yaml from kinetic also works in noetic?
motor_node params dont look exactly the same as before when they were ROS params - so that might be a point of confusion

Comments?

JanezCim · 2021-10-01T09:09:46Z

@rohbotics I implemented per-param correction and added launch checking (launch checking happenes only if --debug is added into command)

Can you re-review and test if it works for you :) Thanks!

MoffKalast

From what I've tested it out both the --debug and regular runs work without issues, and it handles the old yamls as it should.

I've had a few misgivings about a few things, but after thinking them through I mostly agree on how it's set up.

There's just one minor thing:

sonars: None
# sonars: 'pi_sonar_v1' # Use this to enable sonars

    # We only support 1 version of the Sonars right now
    if conf["sonars"] == "pi_sonar_v1":
        conf["sonars"] = True
    else:
        conf["sonars"] = False

    <group if="$(arg sonars_installed)">
        <node pkg="pi_sonar" type="pi_sonar" name="pi_sonar"/>
    </group>

This setup is somewhat inconsistent for either having sonars as a simple toggle, or for supporting multiple types. Either we go all the way and have it boolean in the yaml as well:

sonars: False
# sonars:  True # Use this to enable sonars

Or we go the other way and support the string type in the launch file, so it's possible to add additional options without messing with the generation script:

    <group if="$(eval arg('sonars') == 'pi_sonar_v1')">
        <node pkg="pi_sonar" type="pi_sonar" name="pi_sonar"/>
    </group>

    <group if="$(eval arg('sonars') == 'pi_sonar_v2')">
        <node pkg="pi_sonar_v2" type="pi_sonar_v2" name="pi_sonar_v2"/>
    </group>

    etc.

I assume we should also get rid of the unused legacy param/base.yaml file with this change right? If we leave it there it's become a possible sink for changes that won't be reflected anywhere and may generate confusion.

JanezCim · 2021-10-06T13:47:16Z

@MoffKalast nice catch! Yeah thinking about it, since we arent super key on deversifying the sonar types i'd change it to bool, what do you think @rohbotics ?

We've decided not to get rid of base.yaml because its still kept for non-user editable "parameters" as mentioned in the design choices readme: https://github.com/UbiquityRobotics/magni_robot/tree/bugfix-oldrobotyaml/magni_bringup

But yeah maybe it would be good just to delete the parameters that have now been moved to robot.yaml so we dont have duplication and dont confuse people. Would that be a good alternative @MoffKalast ?

MatjazBostic · 2021-10-11T12:39:33Z

magni_bringup/scripts/launch_core.py


-    conf = get_yaml(conf_path, default_conf)
+    # print out the whole config if in debug mode
+    if arguments.debug == True:


Just a small thing - I don't understand why would you do this instead of if arguments.debug:

I do this so its a smidge easier to read for humans

…ssive behavior

…ni_robot into bugfix-oldrobotyaml

MoffKalast

Seems good as it is right now, nothing really jumps out at me that I'd comment on, at least in a conceptual sense. We've discussed most parts already anyway. Now it just needs to be used by somebody for a bit to see if anything's amiss.

MatjazBostic

I don't really understand what all new parameters are for, but looks good to me.

mjstn · 2021-12-14T20:17:28Z

magni_bringup/config/default_robot.yaml

+  pid_control:  0
+  drive_type: "standard"
+  wheel_type: "standard"
+  wheel_gear_ratio: 4.294


Very pleased that we have all of the following in here: pid_velocity as 0, drive_type as standard, wheel_gear_ratio as 4.294 AND even pid_control as 0! Thanks!

mjstn · 2021-12-14T20:34:13Z

I feel strongly due to massive history we have with base.yaml that we should keep magni_bringup/param/base.yaml

The file remains but only holds a comment that says 'All parameters have been moved to /etc/robot.yaml'

I state this because we have COUNTLESS entries in forum, on learn pages and email threads and customers themselves holding notes that ALL discuss making changes to base.yaml file.

If we keep base.yaml I guarentee you we save MASSIVE numbers of waste of time queries to us about confusion on base.yaml.

MoffKalast · 2021-12-15T00:00:30Z

The file remains but only holds a comment that says 'All parameters have been moved to /etc/robot.yaml'

I can agree with that. As long as there aren't any actual params left in it that would make it appear as an actual config file.

mjstn · 2021-12-15T00:23:47Z

That is correct. Absolutely no parameters in it, commented out or not.
JUST a comment to help the poor slob (I mean customer)

JanezCim · 2021-12-15T08:51:36Z

The file remains but only holds a comment that says 'All parameters have been moved to /etc/robot.yaml'

Yes that is a smart and easy thing to do. And then we can remove it sometime in the future

JanezCim · 2021-12-15T09:02:50Z

I've added the follwing text:

# All parameters have been moved to /etc/ubiquity/robot.yaml and `magni_bringup/config/default_robot.yaml` 
# How it works: if there are some parameters missing in `/etc/ubiquity/robot.yaml` they are taken individually from `magni_bringup/param/default_robot.yaml`. 
# To get to know the details and explanations about this see the `magni_bringup/README.md`

mjstn · 2021-12-15T09:03:55Z

Thank you for understanding the practical need for this file to help customers out of certain confusion for the customers who are following current docs. This leaving base.yaml is such an easy way to avoid I suspect a great many hours spent here and there over time to first realize what was wrong then tell them EXACTLY what would be great to state simply in that file. This is PURELY support. Sometimes the ideal 'keep it all clean' conflicts with backward compatible suportability, this is that case. Thanks

mjstn · 2021-12-15T09:05:57Z

magni_bringup/README.md

This may likely be the MOST important fix of the entire change! ;-)

Very important indeed :D

mjstn · 2021-12-15T09:08:58Z

magni_bringup/config/default_robot.yaml

  pid_proportional: 5000
-  pid_integral: 7
+  pid_integral: 5


Nitpik: This 'could' be as low as 3 but lets just go with this to not slow down this extremely long awaited change. I checked kinetic-devel and noetic-devel and this is a mess so just leave this and we will do more evaluation and other things later. Never hack some number at the 11th hour, it breaks stuff. We ship only what we have tested.

mjstn · 2021-12-15T09:10:28Z

magni_bringup/scripts/launch_core.py

-                    "max_velocity": str(vc_ang["max_velocity"]),
-                    "has_acceleration_limits": str(vc_ang["has_acceleration_limits"]),
-                    "max_acceleration": str(vc_ang["max_acceleration"]),
+


Interesting use of this 'mot_cont' to make the reading here easier. Nice. I think that was pre-existing so I assume it works.

It would also be extremely nice if someone told me they have tested this on their actual robots and it works. I have tested on mine, but you never know.

mjstn · 2021-12-15T09:11:26Z

magni_bringup/scripts/launch_core.py

-        conf["sonars"] = False
+    # print out the whole config if in debug mode
+    if arguments.debug == True:
+        print("DEUBG: Full content of the applied config:")


Cleanup: Is this really needed? This may be a debug artifact

No, "debug" is parameter you can run the launch_core.py with to get some usefull info out of it. Usefull in the future

mjstn · 2021-12-15T09:13:33Z

magni_bringup/scripts/launch_core.py

        # get camera extrinsics
        path1 = (
-            "~/.ros/extrinsics/camera_extrinsics_%s.yaml" % conf["raspicam"]["position"]
+            "~/.ros/extrinsics/camera_extrinsics_%s.yaml" % conf["raspicam_position"]


has somebody thought through this 'symantic' change? It only 'slightly' makes my 'spidy sense' thing that there is room for generation of a bug in this sort of synantic change.

This change MUST be tested by our test group. Please include a test in the test plan.

Not tested with our test group yet.

If the user sets (kinetic way)

raspicam: {"position" : "forward"}

instead of (noetic way)

raspicam_position: 'forward'

The raspicam_position forward will be taken from src/magni_robot/magni_bringup/config/default_robot.yaml thus making the robot not chrash, just the cam position will not be what user has set.

Maybe also here i could add a comment in /etc/ubiquity/robot.yaml that the param has changed from raspicam: {"position" : "forward"} to raspicam_position: 'forward'. Same thing then needs to be done for lidar

mjstn · 2021-12-15T09:17:24Z

I do not see myself as an official 'reviewer' but in this light, 'I Approve'. Thanks all

mjstn · 2021-12-15T09:32:29Z

IMPORTANT but SIMPLE: I think there are already a couple things such as raspicam position and so on that remind me that we really need at the very start of the default_robot.yaml file to have a comment line like this:

File format 2.0

If this is present we then can modify douments like the raspicam setup learn page and so on and say that if they see top of the file that the format is 2.0 or greater use these lines, else use older format lines.

This is rather important frankly. Lets lower the impact of this change by outguessing obvious issues of the future. Thanks

GreTimotej · 2021-12-16T13:18:49Z

After todays test it seemed good to me.

JanezCim · 2021-12-16T13:33:08Z

If this is present we then can modify douments like the raspicam setup learn page and so on and say that if they see top of the file that the format is 2.0 or greater use these lines, else use older format lines.

@mjstn i can do that, but long term id say that the way to go with these things would be that we have a documentation system that suppors the box through which the user can choose a version/distro of the documentation they would like to see. Eg. from readthedocs for ROS2:

This would super simplify our documentations and make it a bit more future proof IMO - since you cant be forever adding cases for each new version to the same .md file

rohbotics · 2021-12-23T19:09:48Z

I think this is ok for now, we will do a further iteration with more generic sensor handling.

JanezCim added 3 commits September 9, 2021 14:25

added color to printout

4658131

lidar and camera enabled with one-level robot.yaml

6ead605

converted whole robot.yaml into one-level thing for clearness

797ef49

JanezCim requested a review from rohbotics September 9, 2021 13:22

added extrinsics install rules

4bbf502

JanezCim mentioned this pull request Sep 10, 2021

magni_description missing install rules #175

Closed

JanezCim added 3 commits September 21, 2021 10:18

removed extrinsics rules

e9cee35

Added per param robot.yaml corection, robot.yaml revert

c2acd93

added launch checking, removed a small error with recursive dict reading

589ea05

readme update

69f3911

JanezCim mentioned this pull request Oct 1, 2021

Adding updated parameter to match recent ubiquity_motor and also addi… #178

Closed

JanezCim requested review from MoffKalast and MatjazBostic October 1, 2021 10:23

MoffKalast reviewed Oct 6, 2021

View reviewed changes

sonars config changed to bool

8edd446

JanezCim requested a review from MoffKalast October 11, 2021 10:14

MatjazBostic approved these changes Oct 11, 2021

View reviewed changes

MoffKalast requested a review from mjstn October 11, 2021 19:48

mjstn and others added 7 commits October 12, 2021 00:36

Adding new motor node and firmware parameters

e857e7b

Change base.yaml gear ratio and max pwm back to good values

79724cf

Back down pid_velocity to 0 and pid_integral to 5, that is less aggre…

39b4811

…ssive behavior

Adding back in serial port and other serial params

10f3a6e

Put all prams form base to robot yaml, deleted base yaml

4649e95

Merge branch 'bugfix-oldrobotyaml' of github.com:UbiquityRobotics/mag…

aeb477a

…ni_robot into bugfix-oldrobotyaml

Adding new serial params into robot yaml, deleted base yaml after merge

6677143

JanezCim requested a review from JanLipovsek October 15, 2021 12:33

JanezCim requested review from MoffKalast, MatjazBostic, mjstn and rohbotics and removed request for rohbotics November 29, 2021 19:58

MoffKalast approved these changes Dec 6, 2021

View reviewed changes

MatjazBostic approved these changes Dec 10, 2021

View reviewed changes

mjstn reviewed Dec 14, 2021

View reviewed changes

Added redirection comments into base.yaml

5f36d8f

base.yaml corection

ff04188

mjstn reviewed Dec 15, 2021

View reviewed changes

Merge branch 'noetic-devel' into bugfix-oldrobotyaml

f3299b1

mjstn reviewed Dec 15, 2021

View reviewed changes

JanezCim requested a review from GreTimotej December 15, 2021 09:30

JanezCim merged commit 24f8a47 into noetic-devel Dec 24, 2021

JanezCim deleted the bugfix-oldrobotyaml branch February 1, 2022 15:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Slightly improved architecture of robot.yaml #177

Slightly improved architecture of robot.yaml #177

JanezCim commented Sep 9, 2021 •

edited

Loading

rohbotics commented Sep 20, 2021

JanezCim commented Sep 21, 2021 •

edited

Loading

JanezCim commented Oct 1, 2021

MoffKalast left a comment

JanezCim commented Oct 6, 2021

MatjazBostic Oct 11, 2021

JanezCim Oct 11, 2021

MoffKalast left a comment •

edited

Loading

MatjazBostic left a comment

mjstn Dec 14, 2021 •

edited

Loading

mjstn commented Dec 14, 2021 •

edited

Loading

MoffKalast commented Dec 15, 2021

mjstn commented Dec 15, 2021

JanezCim commented Dec 15, 2021

JanezCim commented Dec 15, 2021 •

edited

Loading

mjstn commented Dec 15, 2021

mjstn Dec 15, 2021

JanezCim Dec 15, 2021

mjstn Dec 15, 2021

mjstn Dec 15, 2021

JanezCim Dec 15, 2021

mjstn Dec 15, 2021

JanezCim Dec 15, 2021

mjstn Dec 15, 2021

JanezCim Dec 15, 2021 •

edited

Loading

mjstn commented Dec 15, 2021

mjstn commented Dec 15, 2021

GreTimotej commented Dec 16, 2021

JanezCim commented Dec 16, 2021 •

edited

Loading

rohbotics commented Dec 23, 2021

Slightly improved architecture of robot.yaml #177

Slightly improved architecture of robot.yaml #177

Conversation

JanezCim commented Sep 9, 2021 • edited Loading

rohbotics commented Sep 20, 2021

JanezCim commented Sep 21, 2021 • edited Loading

JanezCim commented Oct 1, 2021

MoffKalast left a comment

Choose a reason for hiding this comment

JanezCim commented Oct 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MoffKalast left a comment • edited Loading

Choose a reason for hiding this comment

MatjazBostic left a comment

Choose a reason for hiding this comment

mjstn Dec 14, 2021 • edited Loading

Choose a reason for hiding this comment

mjstn commented Dec 14, 2021 • edited Loading

MoffKalast commented Dec 15, 2021

mjstn commented Dec 15, 2021

JanezCim commented Dec 15, 2021

JanezCim commented Dec 15, 2021 • edited Loading

mjstn commented Dec 15, 2021

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

JanezCim Dec 15, 2021 • edited Loading

Choose a reason for hiding this comment

mjstn commented Dec 15, 2021

mjstn commented Dec 15, 2021

File format 2.0

GreTimotej commented Dec 16, 2021

JanezCim commented Dec 16, 2021 • edited Loading

rohbotics commented Dec 23, 2021

JanezCim commented Sep 9, 2021 •

edited

Loading

JanezCim commented Sep 21, 2021 •

edited

Loading

MoffKalast left a comment •

edited

Loading

mjstn Dec 14, 2021 •

edited

Loading

mjstn commented Dec 14, 2021 •

edited

Loading

JanezCim commented Dec 15, 2021 •

edited

Loading

JanezCim Dec 15, 2021 •

edited

Loading

JanezCim commented Dec 16, 2021 •

edited

Loading