ctu-mrs / mrs_msgs

MRS ROS messages, part of the "mrs_uav_core" package.
https://github.com/ctu-mrs/mrs_uav_core
BSD 3-Clause "New" or "Revised" License
3 stars 5 forks source link

[BUG] Action.msg in the aerial_core folder #2

Closed gsilano closed 2 years ago

gsilano commented 4 years ago

I think there is a bug in the content of the aerialcore_msgs, especially in the Action.msg. It should be

# The Action for command the UAV

#Take off – localize UAV and fly to predefined safe altitude
#Fly to – fly to specified position in working area according to known model of environment
#Guide to – guide UAV to specified position with respect to position of human
#Land – safe landing on starting position with respect to known obstacles

Header header

# Action type
int8 TAKE_OFF=0
int8 FLY_TO=1
int8 GUIDE_TO=2
int8 LAND=3

# The list of points in the path.
mrs_msgs/Reference[] points 

So mrs_msgs/Reference instead of Reference.

Please, check it.

klaxalk commented 4 years ago

I think it is ok since it's within the same package. It builds on my machine...

klaxalk commented 4 years ago

But it does not hurt to add it. However, I am more concerned about the generic name of this specific message. Should be named smth. like AerialCoreAction.msg, @bednarhonza ?

klaxalk commented 4 years ago

We should also consider creating a complementary service, in the sense of:

mrs_msgs/Action
---
bool success
string message
spurnvoj commented 4 years ago

Does it make sense to have it inside of mrs_msgs? We already have repos with msgs for projects like DARPA, MBZIRC or NAKI. So maybe it will be better to have it in a separate repo, something like aerialcore_msgs.

ne 16. 8. 2020 v 23:03 odesílatel Tomáš Báča notifications@github.com napsal:

We should also consider creating a complementary service, in the sense of:

mrs_msgs/Action

bool success string message

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/ctu-mrs/mrs_msgs/issues/2#issuecomment-674577601, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB34JKA7MURJFY3UHNTMTSDSBBCS5ANCNFSM4QA3Z7KA .

klaxalk commented 4 years ago

@spurnvoj that is up to discussion... I guestimated that AerialCore will not require a terribly huge number of messages, so I told @bednarhonza to put it into mrs_msgs. Again, the whole point of having all in one package is making later rosbag processing easier. I think unless it is going to be > #10 new messages, I would keep it in mrs_msgs. But we can split it... which reminds me that I wanted to make a binary within the messages which would publish current commit SHA...

gsilano commented 4 years ago

Ok, let me explain you better. Adding the mrs_msgs ROS messages in MATLAB (I've created an interface in MATLAB to read their content from a rosbag file), it raises an error due to the fact the mrs_msgs folder does not exist as a folder itself. I mean, as mrs_serial, uvdar, and so on. Therefore, I supposed there were a bug or mistake somewhere. Maybe, it is just a matter of how MATLAB organizes the stuff and does not affect the ROS implementation at all. By the way, I'm testing it. If everything is fine, we could think adding the necessary files even here. They might be useful for someone, and could be an adding value for the open source platform (mrs_uav_system).

spurnvoj commented 4 years ago

It was bug ;) look on definition of other msgs that include msg types from the same mrs_msgs pkg, for example this one: https://github.com/ctu-mrs/mrs_msgs/blob/2664a96b6bdb032fc2d9a4c64b4f28b6593eb135/msg/uav_managers/ControlManagerDiagnostics.msg#L15

po 17. 8. 2020 v 6:43 odesílatel Giuseppe Silano notifications@github.com napsal:

Ok, let me explain you better. Adding the mrs_msgs ROS messages in MATLAB (I've created an interface in MATLAB to read their content from a rosbag file), it raises an error due to the fact the mrs_msgs folder does not exist as a folder itself. I mean, as mrs_serial, uvdar, and so on. Therefore, I supposed there were a bug or mistake somewhere. Maybe, it is just a matter of how MATLAB organize the stuff and does not affect the ROS implementation at all.

By the way, I'm testing it. If everything is fine, we could think adding the necessary files even here. They might be useful for someone, and could be an adding value for the open source platform (mrs_uav_system).

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/ctu-mrs/mrs_msgs/issues/2#issuecomment-674651807, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB34JKA47ECCEPGJITM2BFLSBCYNXANCNFSM4QA3Z7KA .

gsilano commented 4 years ago

The same bug is also present in msg/uav_managers/TrajectoryReference.msg and msg/uav_managers/PathReference.msg.

klaxalk commented 4 years ago

@spurnvoj this does not imply that it has to be like that... since it did not matter to this day with the mentioned TrajectoryReference.msg, etc.

@gsilano So does adding _mrsmsgs fixes that for Matlab? Because that does have anything to do with the organizing structure of folders within the package. _mrsmsgs is just the name of the package, where the message is defined.

gsilano commented 4 years ago

Yes, adding mrs_msgs fixes the issue in my case. Some other changes and setup files (i.e., package.xml) are required, but this is strictly related to how MATLAB organizes the file, in particular how the Robotics System Toolbox works.

klaxalk commented 4 years ago

Ok, could you suggest the changes into the _aerialcore branch? I tied it up to the Github issue.

spurnvoj commented 4 years ago

@spurnvoj this does not imply that it has to be like that... since it did not matter to this day with the mentioned TrajectoryReference.msg, etc.

@klaxalk interesting how the documentation of TrajectoryReference.msg differs from the original one. See that in the documentation the points part of the message has mrs_msgs prefix included (mrs_msgs/Reference[]).

spurnvoj commented 2 years ago

@gsilano can we close this one?

gsilano commented 2 years ago

Yes, definitively. I figure out the problem using log files, i.e., rostopic echo /TOPIC_NAME >> log.tx, and then a MATLAB script to generate a MAT.file on them. Anyone who is interesting can just send me an email.