Discuss the possibility to change the signature of the get methods in SensorBridge about bipedal-locomotion-framework HOT 14 OPEN

In that case, you don't want to read from the robot all the times because it is pretty expensive. Hence, I think it is better to have fine control on when those readings need to be updated.

I totally agree that it make sense to unify where the reading of the encoders is done, especially if all the use are happening in the same thread, but just to clarify: RemoteControlBoard::getEncoders is not expensive at all, as it is just reading from an internal buffer that is updated every time new data is received from the robot.

from bipedal-locomotion-framework.

@S-Dafarra answered:

I like the idea. On the other hand, the interfaces so far, including the original Yarp one, assume that no memory is own. If we want to change this, then I would suggest creating a struct containing all the possible data that can be retrieved by the bridge. Each one of the get* would return some reference to the underlying structure. The nice point of this is that it would be possible to write an advance method filling up this struct. The problem is that this struct should be implementation dependent. It would require a lot of copies if we define it with Eigen objects. Maybe it would be possible to do that with the Vector::Ref, but I am not sure.

The problem here is that the get* works only if the user defines some configuration parameters.

from bipedal-locomotion-framework.

@S-Dafarra answered:

I like the idea. On the other hand, the interfaces so far, including the original Yarp one, assume that no memory is own. If we want to change this, then I would suggest creating a struct containing all the possible data that can be retrieved by the bridge. Each one of the get* would return some reference to the underlying structure. The nice point of this is that it would be possible to write an advance method filling up this struct. The problem is that this struct should be implementation dependent. It would require a lot of copies if we define it with Eigen objects. Maybe it would be possible to do that with the Vector::Ref, but I am not sure.

The problem here is that the get* works only if the user defines some configuration parameters.

So we would need a sort of isValid() method?

from bipedal-locomotion-framework.

Which the:

Eigen::Ref<const Eigen::VectorXd>  getJointPositions(double* receiveTimeInSeconds);

how would you handle mutual exclusion, if necessary?
With the usual signature, the caller passes a buffer to the SensorBridge implementation, and if the implementation needs to copy the output data from a shared buffer, it can just have a lock_guard in the body of the get function, and that's it. If instead the caller gets a raw buffer from SensorBridge, how can we enforce that the returned buffer is not in some invalid state when the called tried to copy/use it?

from bipedal-locomotion-framework.

Furthermore, how do you plan to implement this? With the usual signature, the buffer passed by the caller is then forwarded to the YARP methods, but with this new signature you need to create at some point some temporary buffer?

from bipedal-locomotion-framework.

@traversaro

Which the:

Eigen::Ref<const Eigen::VectorXd>  getJointPositions(double* receiveTimeInSeconds);

how would you handle mutual exclusion, if necessary?
With the usual signature, the caller passes a buffer to the SensorBridge implementation, and if the implementation needs to copy the output data from a shared buffer, it can just have a lock_guard in the body of the get function, and that's it. If instead the caller gets a raw buffer from SensorBridge, how can we enforce that the returned buffer is not in some invalid state when the called tried to copy/use it?

That's true I didn't think about it because right now we are assuming that only one thread is used. Indeed the feedback from the sensors are all retrieved with
https://github.com/dic-iit/bipedal-locomotion-framework/blob/defb6ba269217235957d7b8b7674c641fd652217/src/RobotInterface/YarpImplementation/src/YarpSensorBridge.cpp#L127-L140

Furthermore, how do you plan to implement this? With the usual signature, the buffer passed by the caller is then forwarded to the YARP methods, but with this new signature you need to create at some point some temporary buffer?

The Yarp implementation already stores the data in a buffer. Please read the readControlBoardInterface function.
https://github.com/dic-iit/bipedal-locomotion-framework/blob/defb6ba269217235957d7b8b7674c641fd652217/src/RobotInterface/YarpImplementation/include/BipedalLocomotion/RobotInterface/YarpSensorBridgeImpl.h#L1419
So all the data are stored in the controlBoardRemapperMeasures.jointPositions vector. Once the user asks for the jointPositions the content of the vector is copied in an Eigen object.
https://github.com/dic-iit/bipedal-locomotion-framework/blob/defb6ba269217235957d7b8b7674c641fd652217/src/RobotInterface/YarpImplementation/src/YarpSensorBridge.cpp#L268-L275

from bipedal-locomotion-framework.

So all the data are stored in the controlBoardRemapperMeasures.jointPositions vector. Once the user asks for the jointPositions the content of the vector is copied in an Eigen object.

Ah, I see. I always discourage this kind of "copy from buffer to buffer" style, but as long as you always call the get methods in the same thread were you call the advance, then it make sense.

from bipedal-locomotion-framework.

Just to resume this discussion.
Thanks to the advance we read the data from the robot. If the advance returns false something went wrong.
So at this point, I think that the getJointPositions should just return a const reference to an array already stored in SensorBridge

To be compliant with future possible multi-thread usage we may think to add mutex in the get and in the advance

cc @prashanthr05 @traversaro @S-Dafarra @isorrentino

from bipedal-locomotion-framework.

To be compliant with future possible multi-thread usage we may think to add mutex in the get and in the advance

I think this is still not enough. You can lock the mutex in the get, but then if you store the reference in a local variable, you can still access the buffer without having the mutex locked, e.g

foo.advance();
auto ref = foo.get();
ref[2] = 7.0; //This may happen in a separate thread and the mutex is not locked.

I think that the only solution for that case would be "no solution". You avoid any lock (that are super expensive by the way) and you simply rely on the user being careful, although is risky.

from bipedal-locomotion-framework.

foo.get() should return a cost object so the user cannot change it, right?

e.g.

Eigen::Ref<const Eigen::VectorXd>  getJointPositions(double* receiveTimeInSeconds);

from bipedal-locomotion-framework.

foo.get() should return a cost object so the user cannot change it, right?

e.g.

Eigen::Ref<const Eigen::VectorXd>  getJointPositions(double* receiveTimeInSeconds);

Yes, good point. Two concurrent reads are not a problem. But a concurrent read and write are still a problem: if you call advance while reading ref you might end up in an inconsistent state.

from bipedal-locomotion-framework.

Yes, good point. Two concurrent reads are not a problem. But a concurrent read and write are still a problem: if you call advance while reading ref you might end up in an inconsistent state.

That's true this is the reason why we should add a mutex. Even now if you call getJointPositions while another thread calls advance you might get an inconsistent state.

I think that the problem of concurrency is still there even if we do not change the signature.

The reasons why I would like to change the signatures are the follows:

1. we can nest functions ‘foo(getJoints())’
2. We avoid to copy the vectors twice. Once in advance() and once in get()
3. I like if the get function returns what you get ( this is my personal flavour)
4. Currently we have to pass an already reseized vector to the gets (here we may use GenericVector but at this point I would return a const ref to GenericVector) #185

from bipedal-locomotion-framework.

We avoid to copy the vectors twice. Once in advance() and once in get()
I like if the get function returns what you get ( this is my personal flavour)
Currently we have to pass an already reseized vector to the gets (here we may use GenericVector but at this point I would return a const ref to GenericVector) #185

IMO, we could avoid maintaining internal buffers in the YarpSensorBridge and remove the advanceable, so that the appropriate read is done only when the user calls a getter. This would clear points 2 and 3.
This was don for the YarpCameraBridge (unmerged) in #94 (comment) (Related issue #109).

In this case, the use of mutex will also be justified and lead to reduced resource clash.

from bipedal-locomotion-framework.

I think that the problem of concurrency is still there even if we do not change the signature.

Yes, indeed. But at least right now it is easier to handle. You know that you cannot call advance and get from two different threads. With the other signature, you cannot even access the data from a different thread. For example

foo.advance(); \\thread1
foo.getPositions(buffer); \\thread1
doSomethingWithBuffer(buffer); \\thread2

is ok, while

foo.advance(); \\thread1
auto buffer = foo.getPositions(); \\thread1
doSomethingWithBuffer(buffer); \\thread2

may still crash because you may read buffer from thread2 while thread1 attempts to write it.

This is the mutual exclusion problem we have to address. I think that we may overcome it if we output a "lockable" reference, that is a reference that can be locked and unlocked in order to prevent these situations.

The reasons why I would like to change the signatures are the follows:

1. we can nest functions ‘foo(getJoints())’
2. We avoid to copy the vectors twice. Once in advance() and once in get()
3. I like if the get function returns what you get ( this is my personal flavour)
4. Currently we have to pass an already reseized vector to the gets (here we may use GenericVector but at this point I would return a const ref to GenericVector) #185

I think they are indeed reasonable.

IMO, we could avoid maintaining internal buffers in the YarpSensorBridge and remove the advanceable, so that the appropriate read is done only when the user calls a getter.

Note that even in this case we would may have the same mutual exclusion problem of the example above. Plus, I would avoid that because we may need to get the joint positions from multiple positions in the code. In that case, you don't want to read from the robot all the times because it is pretty expensive. Hence, I think it is better to have fine control on when those readings need to be updated.

from bipedal-locomotion-framework.