roboticslab-uc3m / kinematics-dynamics

Kinematics and dynamics solvers and controllers.
https://robots.uc3m.es/kinematics-dynamics/
GNU Lesser General Public License v2.1
19 stars 12 forks source link

CartesianControlServer stream FK #102

Closed jgvictores closed 6 years ago

jgvictores commented 7 years ago

CartesianControlServer stream FK

jgvictores commented 7 years ago

@PeterBowman Before merging 63f105a5758197eda0db852e3af0121b65639b4a I'd like your opinion on:

  1. Maybe a --fkPeriod parameter to establish the frequency? Different parameter name proposal (e.g. --msStateOut?
  2. Most importantly, should the port always be open? Maybe we could avoid opening the port if --fkPeriod 0? Or would that be bad because we may want the port as some kind of dependency (and all the if clauses would make it a bit messy)?
PeterBowman commented 7 years ago

@jgvictores I'm worried about race conditions, perhaps we should guard the iCartesianControl handle behind a mutex/semaphore. Have you tested this code while issuing simultaneous RPC commands? Maybe I'm misunderstanding the concept of locks/races and these considerations pertain to the implementors of ICartesianControl, that is BasicCartesianControl for such instance, and the actual thread guards should be considered in those subdevices instead. Whether the port should be always left open might find an answer through previous questions.

Regarding the name proposal: unless we expect to publish more data types (i.e. not just FK), I'm OK with --fkPeriod.

jgvictores commented 7 years ago

@PeterBowman You're right about your concerns... maybe some semaphores would be good.

PeterBowman commented 7 years ago

Also, I wonder if CartesianControlClient should (try to) open a receiver port and buffer incoming data, just like YARP's remote_controlboard does (state:i and stateExt:i).

BTW I'd rebase issue201fkStream on current develop before resuming work or merging.

jgvictores commented 7 years ago

Also, I wonder if CartesianControlClient should (try to) open a receiver port and buffer incoming data, just like YARP's remote_controlboard does (state:i and stateExt:i).

Makes sense.

BTW I'd rebase issue201fkStream on current develop before resuming work or merging.

:+1:

jgvictores commented 6 years ago

As commented with @PeterBowman latest changes (even folder name changes) are at develop. Should consider rebase or even posible re-impementation before merging.

PeterBowman commented 6 years ago

@jgvictores I rebased your changes on develop (30c955f), now I'm working on fix-102-fkstream-rebased. You may want to delete issue201fkStream if not needed anymore.

Support for --fkPeriod was added in 1300cdb.

Most importantly, should the port always be open?

By now, I'll stick with yes as the answer unless usage proves that we may incur in concurrency issues. A short glimpse at yarp::dev::ControlBoardWrapper's code revealed that they are actually not using semaphores to guard all those interface handles. Well, I found a lonely mutex, but it's not used anywhere.

I expected to see something like this used massively (ref):

yarp::os::Semaphore rpcDataMutex; // mutex to avoid concurrency between more
                                  // clients using rppc port

However, YARP folks call it exclusively whenever a memset is involved for multiple-joint data shuffling.

Edit: now working on the CartesianControlClient part.

PeterBowman commented 6 years ago

Maybe we could avoid opening the port if --fkPeriod 0? Or would that be bad because we may want the port as some kind of dependency (and all the if clauses would make it a bit messy)?

By now, I'll stick with yes as the answer unless usage proves that we may incur in concurrency issues.

Actually, it's quite simple to achieve and could prove useful if latency is an issue (keep in mind that there'll be a loop computing FK every few milliseconds). The client device may easily fall back to RPC if there is no /state:o port.