Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Orientation enum name #342

Closed
wants to merge 18 commits into from
Closed

Conversation

TSC21
Copy link
Member

@TSC21 TSC21 commented Jul 16, 2015

  • Added names to orientation list;
  • Added functions: one to retrieve the name when having the enum, other to retrieve the number when having the name.

@TSC21 TSC21 force-pushed the orientation_enum_name branch 2 times, most recently from f895673 to 3007839 Compare July 16, 2015 21:16
/**
* @brief Retrieve sensor orientation number from alias name.
*/
static int orientation_from_str(std::string sensor_orientation);
Copy link
Member

Choose a reason for hiding this comment

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

Change std::string sensor_orientation to const std::string &sensor_orientation.

Also this function should be capable convert numeric value of enum (e.g. strtol() then check & return).
That needed for backward compatibility.

Copy link
Member Author

Choose a reason for hiding this comment

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

Change std::string sensor_orientation to const std::string &sensor_orientation.

Why? And why not also Eigen::Quaterniond sensor_orientation_matching?

Also this function should be capable convert numeric value of enum (e.g. strtol() then check & return).

Convert to what? MAV_SENSOR_ORIENTATION orientation is supposed to be a number, which is used as an index to return: orientation name in case of UAS::str_sensor_orientation(); and orientation values in case of UAS::sensor_orientation_matching(). And orientation_from_str() was created so we can return the orientation number/index if we give the orientation name. So I think we have all the functionalities we need.

Copy link
Member

Choose a reason for hiding this comment

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

Why?

Because this reduces object copying. What you want change in UAS::sensor_orientation_matching()?

strtol()

Because there may be lots of configs with number representation. Why not just simply try to fall back to old behavior?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because this reduces object copying. What you want change in UAS::sensor_orientation_matching()?

This I undestand, but:

Because there may be lots of configs with number representation. Why not just simply try to fall back to old behavior?

... I can't. Can you code an example please?

@@ -37,7 +37,7 @@ class DistanceSensorItem {
send_tf(false),
sensor_id(0),
field_of_view(0),
orientation(-1),
orientation("LOCAL_NED"),
Copy link
Member

Choose a reason for hiding this comment

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

This make it worse. Nope-nope-nope!
Leave orientation as integer.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, this is supposed to be https://github.com/mavlink/mavros/pull/342/files#diff-2adf7423447a0e5f4880b56161efdda9R51, so how can I init as an integer and use it as a string (which it is)?!

@vooon
Copy link
Member

vooon commented Jul 17, 2015

Only one thing need to change in distance_sensor.cpp: DistanceSensorItem::create_item()

You should pull orientation parameter to temporary std::string, then do lookup and save result in orientation field.

@TSC21
Copy link
Member Author

TSC21 commented Jul 17, 2015

You should pull orientation parameter to temporary std::string, then do lookup and save result in orientation field.

I'm kinda lost so I restored to default (ad7a7b4).
Also I don't know what you pretend here [https://github.com//pull/342#discussion_r34915116].

@TSC21
Copy link
Member Author

TSC21 commented Jul 17, 2015

You should pull orientation parameter to temporary std::string, then do lookup and save result in orientation field.

Don't know what you mean...

uint8_t sensor_id; //!< id of the sensor
bool is_subscriber; //!< this item is a subscriber, else is a publisher
bool send_tf; //!< defines if a transform is sent or not
uint8_t sensor_id; //!< id of the sensor
double field_of_view; //!< FOV of the sensor
tf::Vector3 position; //!< sensor position
Copy link
Member

Choose a reason for hiding this comment

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

That's one field that need to change type: Eigen::Vector3d.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK. But please(!), instead, go to specific lines to change so to orientation handling work ok (that's the purpose of this PR).

vooon pushed a commit that referenced this pull request Jul 18, 2015
vooon added a commit that referenced this pull request Jul 18, 2015
vooon added a commit that referenced this pull request Jul 18, 2015
* orientation_enum_name:
  distance_sensor #342: correct orientation parameter handling.
  lib #342: try to convert numeric value too
  px4_config: adapt to distance_sensor params to new features
  distance_sensor: restructure orientation matching and verification
  lib #342: Added sensor orientation string repr.
@vooon
Copy link
Member

vooon commented Jul 18, 2015

Rebased and merged.

@vooon vooon closed this Jul 18, 2015
@vooon vooon added this to the Version 0.13 milestone Jul 21, 2015
vooon added a commit that referenced this pull request Jul 23, 2015
* master: (87 commits)
  package: comment-out c++03 related flags
  package: Replace YouCompleteMe config with @galou gist
  scripts: fix mavwp
  test: add test cases for new sensor orientation functions
  remove tf1 dep
  lib #319: Remove TF types from UAS
  plugin: param: new message type: ParamValue
  msgs: Move MAV_CMD values to separate msg
  plugin: command: fix build
  fix whitespaces in python scripts
  Added launch file for PX4 posix sitl to launch gcs_bridge node for bridging posix and gazebo
  scripts: mavftp: little speed up by aligning access to payload length
  launch: Add optional log_output arg
  distance_sensor #342: correct orientation parameter handling.
  lib #342: try to convert numeric value too
  Update iris_empty_world_offboard_ctl.launch
  px4_config: adapt to distance_sensor params to new features
  distance_sensor: restructure orientation matching and verification
  lib #342: Added sensor orientation string repr.
  test: fix prerelease building
  ...
@TSC21 TSC21 deleted the orientation_enum_name branch August 2, 2015 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants