search

ROBOTIS-GIT / ros2arduino

This library helps the Arduino board communicate with the ROS2 using XRCE-DDS.
Apache License 2.0
219 stars 43 forks source link

Change HZ from uint32_t to float #32

Closed yottayuan closed 4 years ago

yottayuan commented 5 years ago

Suggest Change ros2.cpp and ros2.hpp>>void ros2::Node::createWallFreq(float hz, CallbackFunc callback, void callback_arg, PublisherHandle pub),to allow Freq below 1. Signed-off-by: yottayuan yuantingwork@163.com

OpusK commented 5 years ago

Hi, @yottayuan

Thanks for your contribution. The code you've modified doesn't meet your intended purpose (set a period less than 1ms). The createWallFreq () function calls the createWallTimer () function, which uses milliseconds. ROS2's API also uses millisecond units.

Do you need to use microseconds? If so, we need to modify the internal code quite a bit.

yottayuan commented 5 years ago

Hi, @yottayuan

Thanks for your contribution. The code you've modified doesn't meet your intended purpose (set a period less than 1ms). The createWallFreq () function calls the createWallTimer () function, which uses milliseconds. ROS2's API also uses millisecond units.

Do you need to use microseconds? If so, we need to modify the internal code quite a bit.

Yes,I want to set the Frequency as like 0.2, 0.1.

OpusK commented 5 years ago

@yottayuan

If so, internal functions, including functions, must be changed. This seems to need discussion.

https://github.com/ROBOTIS-GIT/ros2arduino/blob/master/src/ros2/node_handle.hpp#L60

yottayuan commented 5 years ago

@yottayuan

If so, internal functions, including functions, must be changed. This seems to need discussion.

https://github.com/ROBOTIS-GIT/ros2arduino/blob/master/src/ros2/node_handle.hpp#L60

Really very very sorry for my poor English. Make me clearly: My intended purpose is to set a period larger than 1 second. I want to publish topic every 10 seconds. not set a period less than 1ms but set a period bigger than 1 seconds. So I think just change the type from Uint32_t to float will meet my purpose. By the way, set a period less than 1ms to publish topic need a really really good Net Work. I don't need that.

OpusK commented 4 years ago

Hi, @yottayuan

I'm sorry for the late reply. I understand your intentions and I will check and see if there are any problems with the existing behavior.

OpusK commented 4 years ago

Hi, @yottayuan

Sorry for the late reply. I haven't had time to manage this project recently.

As a result of the review, I decided to keep uint32_t because using float can cause various complications. (As ROS2 does)

My intended purpose is to set a period larger than 1 second. I want to publish topic every 10 seconds. not set a period less than 1ms but set a period bigger than 1 seconds.

I didn't respond to this at the time, but I still leave an answer. If this is your purpose, the current uint32_t type is sufficient. As you know, this is because it can represent up to 4,294,967,295ms.

Anyway, thanks for your attention and contribution :)