Fix the force-torque sensor update rate

[mjcarroll]

🦟 Bug fix

Makes the force-torque sensor system properly respect the update_time parameter from the sensor's SDF.

Summary

This bug is introduced by calling ForceTorqueSensor::Update rather that Sensors::Update from the base class. This was causing all of the base class update logic to be skiped and the FT sensor would update every time.

To test, runt he example sensors world

ign gazebo ./src/ign-gazebo/examples/worlds/sensors.sdf    

And verify that the output rate of the f-t sensor is reasonable (10 hz, in this case):

ign topic -e -t /world/sensors/model/force_torque_demo/joint/joint_01/sensor/force_torque/forcetorque 

Note to maintainers: Remember to use Squash-Merge

[chapulina]

This bug is introduced by calling ForceTorqueSensor::Update rather that Sensors::Update from the base class.

I see the same pattern is used for other sensors, but with this pattern, how are we handling custom updates for each sensor type? That's why they override the parent Sensor::Update function, right?

Is there a problem in ign-sensors itself? I'd expect the overriding functions to call the parent function, i.e.

bool ForceTorqueSensor::Update(const std::chrono::steady_clock::duration &_now)
{
    Sensor::Update(_now);
...

[chapulina]

I see the same pattern is used on other sensors, but is the conversion to sec/nsec and back necessary? Can't we just call Update(_info.simTime, false)?

[mjcarroll]

This may be vestigial from switching over how we handled time in Fortress. I can do a follow up to fix all of the locations where we are using this pattern.

[mjcarroll]

Similar to my comment on the main thread, I think we can do away with the time logic, but we still have to cast to the base class in order to get the Update function that we want to use here.

Our two options would be, neither of which are particularly pretty.

      it.second.get()->sensors::Sensor::Update(_info.simTime, false);
      static_cast<sensors::Sensor*>(it.second.get())->Update(_info.simTime, false);

[mjcarroll]

I updated to my preferred call, but I'm open to suggestions.

[mjcarroll]

One other option would be to put using Sensor::Update(..., ...) in each of the derived classes.

[chapulina]

One other option would be to put using Sensor::Update(..., ...) in each of the derived classes.

I think that would be the right thing to do, but I understand it if it's out of the scope of this PR

[chapulina]

One other option would be to put using Sensor::Update(..., ...) in each of the derived classes.

I was just looking into that, and what's currently happening is that this function:

public: bool Update(const std::chrono::steady_clock::duration &_now, const bool _force);

Calls this one:

public: virtual bool Update(const std::chrono::steady_clock::duration &_now) = 0;

So I think this PR is correct, calling the outermost function. We should do the same for all sensors. And we should rethink whether we want the 2nd function to be public at all; it seems to me that calling it is a mistake, because it only does a subset of what the other function does.

[chapulina]

Follow-up PRs:

• Cleanup update call for non-rendering sensors #1282
• Let derived sensors call Sensor::Update gz-sensors#187
• Improve Sensor::Update call #1283

[mjcarroll]

Ah, so I think I see the issue.

In Sensor, we have:

public: virtual bool Update(const std::chrono::steady_clock::duration &_now) = 0;
public: bool Update(const std::chrono::steady_clock::duration &_now, const bool _force);

Where the argument version calls the virtual version to be implemented by downstream users.

In our systems in ign-gazebo, we generally keep the pointer to the most-derived class, so we don't have access to Sensor::Update that contains the logic that we are most interested in.

[chapulina]

The approach here LGTM, we just need to fix the test:

  /github/workspace/test/integration/force_torque_system.cc:142: Failure
  Expected equality of these values:
    iters
      Which is: 1000
    wrenches.size()
      Which is: 31

[codecov]

Codecov Report

Merging #1159 (a5e0c34) into ign-gazebo6 (ee90892) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head a5e0c34 differs from pull request most recent head 35a3e97. Consider uploading reports for the commit 35a3e97 to get more accurate results

@@             Coverage Diff              @@
##           ign-gazebo6    #1159   +/-   ##
============================================
  Coverage        61.92%   61.93%           
============================================
  Files              275      275           
  Lines            22457    22457           
============================================
+ Hits             13907    13908    +1     
+ Misses            8550     8549    -1     

Impacted Files	Coverage Δ
src/systems/force_torque/ForceTorque.cc	73.72% <100.00%> (ø)
src/EntityComponentManager.cc	87.15% <0.00%> (+0.12%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ee90892...35a3e97. Read the comment docs.