Apply suggestions from code review

Author: urfeex

include/ur_client_library/primary/primary_client.h

@@ -20,24 +20,27 @@ class PrimaryClient
 {
 public:
   PrimaryClient() = delete;
-  PrimaryClient(comm::URStream<PrimaryPackage>& stream);
+  PrimaryClient(const std::string& robot_ip, comm::INotifier& notifier);
   ~PrimaryClient();
 
   /*!
    * \brief Adds a primary consumer to the list of consumers
    *
    * \param primary_consumer Primary consumer that should be added to the list
    */
-  void addPrimaryConsumer (std::shared_ptr<AbstractPrimaryConsumer> primary_consumer);
+  void addPrimaryConsumer (std::shared_ptr<comm::IConsumer<PrimaryPackage>> primary_consumer);
 
   /*!
    * \brief Remove a primary consumer from the list of consumers
    *
    * \param primary_consumer Primary consumer that should be removed from the list
    */
-  void removePrimaryConsumer(std::shared_ptr<AbstractPrimaryConsumer> primary_consumer);
+  void removePrimaryConsumer(std::shared_ptr<comm::IConsumer<PrimaryPackage>> primary_consumer);
   void start();
 
+  /*!
+   * \brief Retrieves previously raised error codes from PrimaryClient. After calling this, recorded errors will be deleted.
+   */
   std::deque<ErrorCode> getErrorCodes();
 
 private:
@@ -51,7 +54,7 @@ class PrimaryClient
 
   comm::INotifier notifier_;
 
-  comm::URStream<PrimaryPackage>& stream_;
+  comm::URStream<PrimaryPackage> stream_;
   std::unique_ptr<comm::URProducer<PrimaryPackage>> prod_;
   std::unique_ptr<comm::Pipeline<PrimaryPackage>> pipeline_;
 

include/ur_client_library/ur/ur_driver.h

@@ -536,7 +536,7 @@ class UrDriver
   bool checkCalibration(const std::string& checksum);
 
   /*!
-   *  \brief Retrieves error codes from PrimaryClient.
+   *  \brief Retrieves previously raised error codes from PrimaryClient. After calling this, recorded errors will be deleted.
    *
    *  \returns list of error codes
    *

src/primary/primary_client.cpp

@@ -6,8 +6,8 @@ namespace urcl
 {
 namespace primary_interface
 {
-PrimaryClient::PrimaryClient(comm::URStream<PrimaryPackage>& stream)
-  : stream_(stream)
+PrimaryClient::PrimaryClient(const std::string& robot_ip, comm::INotifier& notifier)
+  : stream_(robot_ip, UR_PRIMARY_PORT)
 {
   prod_.reset(new comm::URProducer<PrimaryPackage>(stream_, parser_));
 
@@ -32,15 +32,16 @@ PrimaryClient::~PrimaryClient()
 void PrimaryClient::start()
 {
   URCL_LOG_INFO("Starting primary client pipeline");
+  pipeline_->init();
   pipeline_->run();
 }
 
-void PrimaryClient::addPrimaryConsumer(std::shared_ptr<AbstractPrimaryConsumer> primary_consumer)
+void PrimaryClient::addPrimaryConsumer(std::shared_ptr<comm::IConsumer<PrimaryPackage>> primary_consumer)
 {
   multi_consumer_->addConsumer(primary_consumer);
 }
 
-void PrimaryClient::removePrimaryConsumer(std::shared_ptr<AbstractPrimaryConsumer> primary_consumer)
+void PrimaryClient::removePrimaryConsumer(std::shared_ptr<comm::IConsumer<PrimaryPackage>> primary_consumer)
 {
   multi_consumer_->removeConsumer(primary_consumer);
 }

src/ur/ur_driver.cpp

@@ -74,7 +74,7 @@ urcl::UrDriver::UrDriver(const std::string& robot_ip, const std::string& script_
       new comm::URStream<primary_interface::PrimaryPackage>(robot_ip_, urcl::primary_interface::UR_SECONDARY_PORT));
   secondary_stream_->connect();
 
-  primary_client_.reset(new urcl::primary_interface::PrimaryClient(*primary_stream_));
+  primary_client_.reset(new urcl::primary_interface::PrimaryClient(robot_ip_, notifier_));
 
   non_blocking_read_ = non_blocking_read;
   get_packet_timeout_ = non_blocking_read_ ? 0 : 100;

tests/test_ur_driver.cpp

@@ -381,7 +381,10 @@ TEST_F(UrDriverTest, reset_rtde_client)
 
 TEST_F(UrDriverTest, read_error_code)
 {
+  g_consume_rtde_packages = true;
   g_ur_driver->startPrimaryClientCommunication();
+  // Wait until we actually received a package
+  std::this_thread::sleep_for(std::chrono::milliseconds(1000));
   std::stringstream cmd;
   cmd << "sec setup():" << std::endl << " protective_stop()" << std::endl << "end";
   EXPECT_TRUE(g_ur_driver->sendScript(cmd.str()));
@@ -398,8 +401,8 @@ TEST_F(UrDriverTest, read_error_code)
   ASSERT_EQ(error_codes.at(0).message_code, 209);
   ASSERT_EQ(error_codes.at(0).message_argument, 0);
 
-  // Wait for 5s after PSTOP before clearing it
-  std::this_thread::sleep_for(std::chrono::seconds(5));
+  // Wait for after PSTOP before clearing it
+  std::this_thread::sleep_for(std::chrono::milliseconds(100));
 
   EXPECT_TRUE(g_dashboard_client->commandCloseSafetyPopup());
   EXPECT_TRUE(g_dashboard_client->commandUnlockProtectiveStop());