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

Fix ros2 parameter descriptions and range values #2600

Merged

Conversation

SamerKhshiboun
Copy link
Collaborator

@SamerKhshiboun SamerKhshiboun commented Jan 18, 2023

Tracked by [DSO-18759]

  • Added the correct step value to the Integer Range step value.
  • Added default value to the parameter description section.
  • Added parameter type
  • Did some manipulation on floats before converting them to double, in order to get more precise values Thanks to @maloel suggestion
  • Now, all warnings due to parameters wrong ranges or values are solved.
  • Split long functions into short ones

@SamerKhshiboun SamerKhshiboun marked this pull request as ready for review January 22, 2023 22:02
@SamerKhshiboun
Copy link
Collaborator Author

Don't merge yet.
Might change some other parts related to other warnings (not related to this ticket) but in the same area.

@christian-rauch
Copy link
Contributor

Don't merge yet.

You can convert a PR into a draft (see Convert to draft next to Still in progress? in the top right) to disable merging. Once you are ok with the PR, you can remove the "draft" state.

@SamerKhshiboun SamerKhshiboun marked this pull request as draft January 24, 2023 21:38
@SamerKhshiboun SamerKhshiboun marked this pull request as ready for review January 30, 2023 09:12
option_value = static_cast<T>(sensor.get_option(option));
op_range = sensor.get_option_range(option);
float current_val = sensor.get_option(option);
if(std::is_same<T, double>::value && current_val > 0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

!= 0?

}
else
{
rcl_interfaces::msg::FloatingPointRange range;
range.from_value = double(op_range.min);
range.to_value = double(op_range.max);
range.from_value = static_cast<double>(op_range.min);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe needs conversion too?

@SamerKhshiboun SamerKhshiboun changed the title Fix ros2 sensor controls steps and add control default value to param description Fix ros2 parameter descriptions and range values Jan 31, 2023
float range = option_range.max - option_range.min;
int steps = range / option_range.step;
double step = static_cast<double>(range) / steps;
double rounded_div = std::round((val - option_range.min) / step);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please protect div_by_zero. for both devision operations

Copy link
Collaborator

@Nir-Az Nir-Az Feb 3, 2023

Choose a reason for hiding this comment

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

@SamerKhshiboun we forgot about it before merging please add protection
I wonder why cppcheck didn't shout on it..

Copy link
Collaborator

@Nir-Az Nir-Az left a comment

Choose a reason for hiding this comment

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

Great!

@Nir-Az Nir-Az merged commit 1173504 into IntelRealSense:ros2-development Jan 31, 2023
@SamerKhshiboun SamerKhshiboun deleted the fix_ros2_service branch February 1, 2023 20:58
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.

3 participants