ros-industrial / ros2_canopen

CANopen driver framework for ROS2
https://ros-industrial.github.io/ros2_canopen/manual/rolling/
127 stars 52 forks source link

Documentation for polling is incorrect #198

Open gsalinas opened 9 months ago

gsalinas commented 9 months ago

The description for polling canopen/sphinx/user-guide/cia402-driver.rst / online here reads:

Parameter Type Description
polling bool Enables polling of the drive status. Default: true. If false, period will be used to run a ros2 timer as update loop. If true, the update loop will be triggered by the sync signal and directly executed in the canopen realtime loop. This requires all data processed in the update loop to be PDO, otherwise the loop will get stuck. This can speed reduce processor load significantly though.

However, this description is backwards: polling set to true uses a ros2 timer, and polling set to false uses the sync signal - see node_canopen_base_driver_impl.hpp:

template <class NODETYPE>
void NodeCanopenBaseDriver<NODETYPE>::activate(bool called_from_base)
{
  nmt_state_publisher_thread_ =
    std::thread(std::bind(&NodeCanopenBaseDriver<NODETYPE>::nmt_listener, this));
  emcy_queue_ = this->lely_driver_->get_emcy_queue();
  rpdo_queue_ = this->lely_driver_->get_rpdo_queue();
  if (polling_)
  {
    RCLCPP_INFO(this->node_->get_logger(), "Starting with polling mode.");
    poll_timer_ = this->node_->create_wall_timer(
      std::chrono::milliseconds(period_ms_),
      std::bind(&NodeCanopenBaseDriver<NODETYPE>::poll_timer_callback, this), this->timer_cbg_);
  }
  else
  {
    RCLCPP_INFO(this->node_->get_logger(), "Starting with event mode.");
    this->lely_driver_->set_sync_function(
      std::bind(&NodeCanopenBaseDriver<NODETYPE>::poll_timer_callback, this));
  }

Also, this information is only on the documentation page for Cia402Drivers, and is only present in the rolling docs, but it applies to BaseDrivers as well and it's available in iron too - it'd be helpful if it were documented there.

Thank you!

hellantos commented 9 months ago

@gsalinas thanks for finding that. I will have it updated as soon as possible.