Motorcontrol Refactoring

[gcjurgiel]

After a bit of experimentation I've settled on a structure I hate the least, though it is far from perfect so I'd like your input. For reference, the code I've made is in the motor_controler_interface_update branch.

My first goal is to remove the extensive templating we used last year. It's a powerful and useful tool, but the way it propagated through our entire codebase created the horrible compiler errors we got last year.

I first tried to put both linear (meters) and angular(radians) units into the same class, but that created issues in converting between units in more complex mechanisms (where you can't just multiply by a constant). Furthermore, having both units doesn't always make sense, meaning half the functions would return null.

Instead I separated linear and angular units into their own classes, and made a method to generate a controller in the other unit type for the common case of a linear conversion. In more complex cases, we can make a custom converter classes. It provides additional benefit since we only need to implement the angular class for each motor controllers and we can automatically generate the corresponding linear controller when required.

This structure unfortunately makes templates really tempting since there is lots of duplicate code with only the units changed. And I did write an implementation using templates that I'm tempted to use since I think it avoids the problems of last year. Since we only need to implement motors in radians, the templating will not propagate through the rest of the codebase. This way the templating does what we want, eliminating duplicate code, without the downsides of a more complex implementation.

The specifics of the interface and what actual functions should go in it is also still up for debate, but I think this method of handling units is the best I found so far. What do you all think?

[abheekda1]

Exciting! I'll take a look and contemplate stuff once the jet lag wears off 🥱.

[theVerySharpFlat]

@gcjurgiel I really like this interface! 🎉 It's certainly much more concise than what we had before. I personally don't think templating is worth it, but we can discuss this into further detail.

I gave the code a readover and found only on minor issue. I think LinearPositionController is a virtual class and doesn't have a constructor defined.

IMHO, the interface for getting the conversion should be more like this:

class AngularPositionController {
...
LinearPositionController asLinear(ConversionUnit_t conversion) {
    return AngularAsLinearPositionController(*this, conversion); // or whatever the classname is for the conversion class
}
...
}

I think we can avoid pointers almost entirely as the conversion class handles a tiny amount of data (one reference?) and can be constructed every time the asLinear function is called.

I'm itching to use this API. I think this proves that we can write good code if we think about it a little more.

Thanks for doing all of the work!!! Aiden

[gcjurgiel]

Thanks for looking it over, and I'm glad you like it!

I think that LinearPositionController should remain a virtual class since it's just the interface. AngularAsLinearPositionController does the actual converting. However, this means we can't return a raw LinearPositionController object. The simplest solution is to actually return an AngularAsLinearPositionController object. when I originally wrote this, I purposefully used a pointer to erase the underlying type as a way of hiding the converter class from the user, but I don't remember why I thought that was needed so I'll probably get rid of it.

As for the templates, I'm inclined to agree with you. The structure is easier to understand without it, and it will only introduce more errors. My only hesitance comes from some of non unit specific wrapper I've been messing with. Templates reduce duplicate code, but I'm not sure the wrappers are worth putting time into when most of their functionality can be done by the motor controllers themselves. Any that could actually be useful would really only need to done in angular units, and could be implemented twice if needed.

I'll make the changes I mentioned above and we can talk more once school starts again.

[abheekda1]

I wasn't involved much towards the end of last year but I think this looks very good. I don't see a problem with using smart pointers or references when converting from angular to linear and vice versa. Excited to start implementing this when we get back.

[gcjurgiel]

A proposed modification to the asLinear function, but it has to defined outside the original class.

std::unique_ptr<LinearPositionController> asLinear(std::unique_ptr<AngularPositionController> angularController, 
                                                   units::unit_t<units::compound_unit<units::meters, units::inverse<units::radians>>> conversion) {

  return std::make_unique<AngularAsLinearPositionController>(angularController, conversion);
}

If I do it right, the converter class now takes ownership over the base controller. This prevents undefined behavior from things going out of scope. It also prevent the possibility of creating two objects that control the same motor controller that could be send contradicting signals.

[gcjurgiel]

Thinking about it, Aiden is right, it's probably better to use shared pointers for their flexibility. Our use of abstract classes means that the ownership model isn't always clear. Returning references don't work when the sub-class doesn't own the object being returned, and unique pointers don't work when they do (since we don't want the original to give-up that ownership). Shared pointers can work in both these situations even though it can confuse who "really" controls the object. It also means we will have to watch-out for circular ownership problems, but I don't think that will be an issue.