nobleo / path_tracking_pid

Path Tracking PID offers a tuneable PID control loop, decouling steering and forward velocity
Apache License 2.0
130 stars 37 forks source link

Switched distToSegmentSquared() to proper units. #79

Closed lewie-donckers closed 2 years ago

lewie-donckers commented 2 years ago

Boot Units are go! 🚀 This will be the first PR in a series. I will change a few functions and/or variables per PR. This keeps the changes small and easy to review. Gradually all conversions from (.value()) and to (* units::meter) the strong types will move to the outer edges of the code.

Changed Controller::distToSegmentSquared() to Boost Units.

lewie-donckers commented 2 years ago

Can I just remark that I think we moved to the implementation of Boost Units rather swiftly. In my mind, the discussion was not just about the question "Should we go for either Boost or our own implementation?" but rather "Should we go for a strong-typed unit system, and if we do, how should we do that?". I feel like we haven't even decided on this first part of the question.

Here's a few arguments against:

  • It increases verbosity
  • It increases complexity for newbie developers (of which we have a few)

And it hasn't solved any bugs yet, if I'm not mistaken.

I agree that it is a fun (thought) exercise from a software engineering point of view, but maybe we have more pressing problems that are worth spending this time on?

Okay... Maybe we need a different way for me to propose changes than these PRs. I thought I had 'permission' to continue and that we were all on the same page. Clearly this was not yet the case for this change. Perhaps we should discuss certain changes in a Teams meeting? I do need regular and timely feedback, else I would be blocked.

As to the arguments against it:

Indeed, these changes have not fixed any bugs yet. But that wasn't my assignment. My assignment is to increase code quality. And with a unit of measurement library you can prevent entire classes of errors. The compiler will simply prevent you from doing so.

rokusottervanger commented 2 years ago

I totally hear you. I'm also definitely not saying that we shouldn't do this, but I think we should have an open discussion about this with some stakeholders. Could you set up a meeting? I think @Timple and @Rayman should be involved. Anyone else?

lewie-donckers commented 2 years ago

Closing this PR. Boost units changes can still be seen here: https://github.com/nobleo/path_tracking_pid/compare/cleanup/boost-units-4