search

robotology / yarp-devices-forcetorque

YARP Drivers for various commercial Force Torque sensors.
4 stars 4 forks source link

Feature: Add ftNode device driver to read data from the serial port of wireless receiver #15

Closed yeshasvitirupachuri closed 4 years ago

yeshasvitirupachuri commented 5 years ago

This PR adds a new ftNode device driver, that connects to a yarp serial port device containing data from the wireless receiver of the FTShoes. Additionally, it also modified the ftShoe driver to attach to the ftNode driver and read the FT data.

traversaro commented 5 years ago

I am not sure this repository is the right place where to put this device, as the serial protocol that this device speaks for interacting with the rest of the wireless architecture is still extremely under development, and I would expect that will change a lot in the future.

A more appropriate location for this code perhaps is in the same repo were the other firmware of the other end of the serial communication is stored and developed, so that when the protocol evolves, the two implementation can be updated in parallel, to avoid incompatibility problems and the need to manually synch the two repos in which the repo is ongoing.

What do you think @Yeshasvitvs @DanielePucci @evalli-iit @Gianlucamilani @ludwigthemad ?

On a minor note, I think it would make sense to also implement the yarp::dev::ISixAxisForceTorqueSensors interfaces in the device, as the IAnalogSensors YARP devices are going to be softly deprecated in the future in a favor of the new multiple analog sensors interfaces, see for example https://github.com/robotology/QA/issues/360 .

DanielePucci commented 5 years ago

@traversaro

What do you think @Yeshasvitvs @DanielePucci @evalli-iit @Gianlucamilani @ludwigthemad ?

Agreed!

traversaro commented 5 years ago

Friendly ping @Yeshasvitvs .

fjandrad commented 5 years ago

I will take a look at the review but I got the impression I will be moved to another repo?

A more appropriate location for this code perhaps is in the same repo were the other firmware of the other end of the serial communication is stored and developed, so that when the protocol evolves, the two implementation can be updated in parallel, to avoid incompatibility problems and the need to manually synch the two repos in which the repo is ongoing.

yeshasvitirupachuri commented 5 years ago

I will take a look at the review but I got the impression I will be moved to another repo?

A more appropriate location for this code perhaps is in the same repo were the other firmware of the other end of the serial communication is stored and developed, so that when the protocol evolves, the two implementation can be updated in parallel, to avoid incompatibility problems and the need to manually synch the two repos in which the repo is ongoing.

@fjandrad I am not sure if we had a proper discussion on the location of the code. @traversaro @lrapetti please update me if you discussed this already.

traversaro commented 5 years ago

I personally did not discuss this outside of this issue.

traversaro commented 5 years ago

Friendly ping @Yeshasvitvs @lrapetti : if you prefer not to move this code to another repo, feel free to say so, but I think we should definitly try to avoid code that is actually used for testing in a random PR on a random branch for months, without a clear location.

lrapetti commented 5 years ago

I think we may close this PR and eventually create a dedicated repo for wireless devices given that more devices are going to be create.

This week we should discuss about the software architecture regarding wireless sensors, deciding where to store software should be one of the points to be discussed all togheter @Yeshasvitvs @evalli-iit @Gianlucamilani @ludwigthemad

yeshasvitirupachuri commented 5 years ago

if you prefer not to move this code to another repo, feel free to say so,

I don't have any problem with that.

Given the comment by @fjandrad

I will take a look at the review but I got the impression I will be moved to another repo?

I think the PR review can still be useful when the code is small and relevant to just the FTShoes. However, as mentioned by @lrapetti we have plans to discuss a bit about the software architecture to facilitate wireless nodes in the wearables repo.

traversaro commented 5 years ago

This week we should discuss about the software architecture regarding wireless sensors, deciding where to store software should be one of the points to be discussed all togheter @Yeshasvitvs @evalli-iit @Gianlucamilani @ludwigthemad

Great, then feel free to close this PR if you think it make sense. For what it concerns the review, probably it could make sense to make it when the code is added in the new repo?

yeshasvitirupachuri commented 5 years ago

@traversaro Ok, I will close the PR for now and link the relevant issues after the discussion on the software architecture for the wireless nodes.

CC @fjandrad @lrapetti

yeshasvitirupachuri commented 4 years ago

@DanielePucci In view of the deliverable, I would like to merge this PR to upstream.

yeshasvitirupachuri commented 4 years ago

Thanks @lrapetti

traversaro commented 4 years ago

@DanielePucci In view of the deliverable, I would like to merge this PR to upstream.

It may be worth to at least write a sentence of what changed w.r.t. to the previous discussion of keeping this code in another repo.

traversaro commented 4 years ago

@DanielePucci In view of the deliverable, I would like to merge this PR to upstream.

It may be worth to at least write a sentence of what changed w.r.t. to the previous discussion of keeping this code in another repo.

Thanks, @lrapetti probably provided that sentence.