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

Adds simulation and mockup threads for drilling machine plugin #13

Open
wants to merge 4 commits into
base: rockin
Choose a base branch
from

Conversation

moriarty
Copy link
Contributor

Adds drilling_machine_simulation_thread and drilling_machine_mockup_thread

In simulation mode, the drilling machine the operation is simulated.
In mockup mode, clips functions are exposed but nothing is simulated.

This pull request closes #10. Feedback and re-factoring has been incorporated.

@@ -18,7 +18,7 @@ llsfrb:
status_port: !tcp-port 55501

drilling-machine:
enable: false
mode: mockup
Copy link
Contributor

Choose a reason for hiding this comment

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

In my opinion we can enable the simulation by default. What do you think?

@svenschneider
Copy link
Contributor

Besides my comments this looks good. Please fix or provide feedback.

@moriarty
Copy link
Contributor Author

@svenschneider - feedback has been integrated


void DrillingMachineSimulationThread::loop()
{
fawkes::MutexLocker lock2(&drill_machine_mutex_);
Copy link
Contributor

Choose a reason for hiding this comment

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

This together with the last line in this function leads to a horrible lockup of the complete CFH:

  • The scoped lock is acquired here
  • In the last line the thread goes to sleep, but does not release the lock. Therefore, all calls that also need to lock the mutex will wait.

My proposal is to manually release the lock before sending the thread to sleep.

Additionally, why is the variable here (and in other places) called lock2?

@svenschneider
Copy link
Contributor

Please integrate the feedback, squash the commits appropriately and update the pull request.

When squashing the commits please also update the commit messages. The convention for the subject line is "component: message", where the message is all lower-case and limited to 50 characters. Component is the "top-level" folder of the changed file(s), here component should be "plugins" (within the rockin folder).

@svenschneider
Copy link
Contributor

Any update?

- Adds simulation and mockup threads for drilling machine plugin
- Default Drilling Machine config to simulation
@moriarty
Copy link
Contributor Author

@svenschneider - update, I fixed all the feedback and rebased squashing commits.

I used

git push -f moriarty drill-plugin-sim-rebased:drill-plugin-sim

There are still some comments about locking the mutex. Should I be locking it there?
The value is being read. I don't think we're locking it their in the other device threads.

@svenschneider
Copy link
Contributor

The existing code (hopefully) may work because the variable that is read, and concurrently written from somewhere else, atomically. Therefore, the assumptions for this code being correct are:

  1. This variable is accessible atomically on all hardware platforms (we want to use).
  2. ProtoBuf always maps the variable to the "right" data type.
    If the assumptions don't hold, the reading may result in corrupt data.

Thus, in my opinion the safe way is to lock the mutex for each access (read and write). Could you please check if this has any noticable effect?

@moriarty
Copy link
Contributor Author

@svenschneider I've added the lock(clips_mutex) to both the simulation and real thread.
I did both on this branch.

last_command_msg_.set_command(drill_command);
last_sent_command_timestamp_ = boost::posix_time::microsec_clock::local_time();

logger->log_info("DrillingMachineSim", "Send drill command: %d", drill_command);
Copy link
Contributor

Choose a reason for hiding this comment

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

I just saw that this is "spamming" the terminal. Therefore, I would suggest to use "logger->log_debug([...])".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should I modify that in the other plugins as well? And the non-simulated versions?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, would be nice.

@svenschneider
Copy link
Contributor

Thanks, there is one more minor issue about the logging. When that feedback is integrated, we should be ready to go.

@moriarty
Copy link
Contributor Author

I removed some of the logging and switched some log_debug as suggested.

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.

2 participants