Skip to content
This repository was archived by the owner on Feb 21, 2021. It is now read-only.

[Libs] Makes YML config file description more readable#1070

Merged
fqez merged 7 commits into
JdeRobot:masterfrom
pushkalkatara:feature-lib
Feb 22, 2018
Merged

[Libs] Makes YML config file description more readable#1070
fqez merged 7 commits into
JdeRobot:masterfrom
pushkalkatara:feature-lib

Conversation

@pushkalkatara
Copy link
Copy Markdown
Member

@pushkalkatara pushkalkatara commented Feb 20, 2018 •
edited
Loading

Instead of

Server: 1
 or
Server: 2

Changed to

Server: "Ice"
 or
Server: "ROS"

Solves issue #1060

Images to show working -
3

@pushkalkatara
Copy link
Copy Markdown
Member Author

pushkalkatara commented Feb 20, 2018 •
edited
Loading

@fqez . Running Python file successfully loads the YML, but further due to

sudo pip2 install zeroc-ice

i think pip installs wrapper for version 3.7 of Ice but JdeRobot supports 3.6. So after loading the image, error comes -
6
Shall i open this as a separate issue?

fqez
fqez suggested changes Feb 21, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@fqez fqez left a comment

Choose a reason for hiding this comment

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

There's a couple of issues with this PR. Please review them and you are good to go.
Good job!!

Comment thread src/libs/comm_py/comm/laserClient.py.only-ice.in Outdated
server=1
elif server == "ROS":
server=2
else server = 0
Copy link
Copy Markdown
Member

@fqez fqez Feb 21, 2018

Choose a reason for hiding this comment

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

you did miss a colon here else: server = 0

Comment thread src/libs/comm_py/comm/laserClient.py.ros.in Outdated
server=1
elif server == "ROS":
server=2
else server = 0
Copy link
Copy Markdown
Member

@fqez fqez Feb 21, 2018

Choose a reason for hiding this comment

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

you did miss a colon here else: server = 0

Actually you missed that colon in every client, please, review it.

Comment thread src/drivers/MAVLinkServer/MAVProxy/uav_viewer_py.yml Outdated
@@ -1,30 +1,30 @@
Camera:
Server: 1 # 0 -> Deactivate, 1 -> Ice , 2 -> ROS
Server: "Ice" # Deactivate, Ice , ROS
Copy link
Copy Markdown
Member

@fqez fqez Feb 21, 2018

Choose a reason for hiding this comment

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

We prefer not to have quotes. As easy as possible for the end user :D Server: "Ice" --> Server: Ice
Also, this should be case insensitive (ice, ICE, IcE, Ice, ROS, ros, RoS, rOS, ...)

Please, change this in every yml file!

@fqez
Copy link
Copy Markdown
Member

fqez commented Feb 21, 2018

did you run pip install zeroc-ice? If so, you maybe have installed ice 3.7 which we doesn't support (we support 3.6) so uninstall it and try to run sudo pip2 install zeroc-ice==3.6.3 instead.

Also we need this feature integrated ASAP (other parts of the project depend on this particular one), so let me know if you can resolve it quickly. If not, I can merge this one in this current state and take care of the remaining problems myself.

Good job anyway! :)

@pushkalkatara
Copy link
Copy Markdown
Member Author

pushkalkatara commented Feb 21, 2018 •
edited
Loading

@fqez Thanks for the help. pip install zeroc-ice==3.6.3 worked without any issues. How to update the documentation here(jderobot-manual5)?
Also i have made the changes. Please review again.
I've added -

  • Removed the string colans. "ROS" ---> ros or "ICE" ---> ice
  • Now the yml supports every type-case. (ice, ICE, icE, Ice, ROS, ros, Ros, rOS etc)
  • Python script changes.

Please review the changes.

fqez
fqez suggested changes Feb 22, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@fqez fqez left a comment

Choose a reason for hiding this comment

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

Last changes, tested by myself and working!

Comment thread src/libs/comm_py/comm/communicator.py.only-ice.in Outdated
ymlNode = self.config.getProperty(prefix)
for i in ymlNode:
if type(ymlNode[i]) is dict and ymlNode[i]["Server"] == 1:
if type(ymlNode[i]) is dict and ymlNode[i]["Server"] == "Ice":
Copy link
Copy Markdown
Member

@fqez fqez Feb 22, 2018

Choose a reason for hiding this comment

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

You forgot the case sensitiviness here!

Comment thread src/libs/comm_py/comm/communicator.py.ros.in Outdated
ymlNode = self.config.getProperty(prefix)
for i in ymlNode:
if type(ymlNode[i]) is dict and ymlNode[i]["Server"] == 1:
if type(ymlNode[i]) is dict and ymlNode[i]["Server"] == "Ice":
Copy link
Copy Markdown
Member

@fqez fqez Feb 22, 2018

Choose a reason for hiding this comment

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

case sensitive!

Comment thread src/libs/comm_py/comm/communicator.py.ros.in Outdated
if type(ymlNode[i]) is dict and ymlNode[i]["Server"] == "Ice":
iceserver = True
if type(ymlNode[i]) is dict and ymlNode[i]["Server"] == 2:
if type(ymlNode[i]) is dict and ymlNode[i]["Server"] == "ROS":
Copy link
Copy Markdown
Member

@fqez fqez Feb 22, 2018

Choose a reason for hiding this comment

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

case sensitive

@pushkalkatara
[Libs] Makes YML config file description more readable
5bcbd22 
Instead of
```
Server: 1
 or
Server: 2
```
Changed to
```
Server: "Ice"
 or
Server: "ROS"
```

Solves issue JdeRobot#1060
@pushkalkatara
Copy link
Copy Markdown
Member Author

pushkalkatara commented Feb 22, 2018

@fqez Made the changes necessary changes. Good to go! 👍
Can you please help me with the pipeline of Python tests. It would help me debug the python comms.

@fqez
Copy link
Copy Markdown
Member

fqez commented Feb 22, 2018

There is a conflict with src/drivers/MAVLinkServer/MAVProxy/uav_viewer_py.yml so I can't merge this one automatically. Is it necessary?
There are some test here https://github.com/JdeRobot/JdeRobot/tree/master/src/libs/comm_py/tests you can check them in order to test your modifications on the comm library.

Solve the conflict, please. :)

@fqez
Copy link
Copy Markdown
Member

fqez commented Feb 22, 2018

Everything is now ok! Very nice job @pushkalkatara. We do appreciate your effort. Merging it, thanks!!

@fqez
Copy link
Copy Markdown
Member

fqez commented Feb 22, 2018

undo the last commit please, you broke it! xD

@pushkalkatara
Copy link
Copy Markdown
Member Author

pushkalkatara commented Feb 22, 2018

Sorry some git issues. Squashing my commits

@fqez fqez merged commit f137a58 into JdeRobot:master Feb 22, 2018
@pushkalkatara
Copy link
Copy Markdown
Member Author

pushkalkatara commented Feb 22, 2018

Thanks for the merge. 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@fqez fqez fqez approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@pushkalkatara @fqez