RSDK-4713 RSDK-5626 MovementSensor

[abe-winter]

What changed

• implement MovementSensor component (mostly copying from Zack's branch)
• incidental: fix issue in handler map where only 1st model implementing an API is registered
• add (privately for now) ResourceRegistration2 a one-liner to implement a resource registration -- this is in movementsensor.cpp for now but could be dropped into registry.hpp if useful in the future

[acmorrow]

I love a good boilerplate elimination, and I have lots of thoughts. Feel free to take or leave what you find useful.

One thing I would suggest, is to split this into two reviews. One to just add the movement sensor under the standard registration mechanics, and then a follow-up to refactor registration if that ends up being considered worthwhile.

[acmorrow]

Let's come up with a better name for this than ...2. If you intend for this to be the canonical way, maybe this should be ResourceRegistration and the existing ResourceRegistration should be ResourceRegistrationBase.

[acmorrow]

I wonder if you could get rid of the CRTP if this wasn't a static member of the template, but a free function. Something like:

template <typename ClientType, typename ServerType, typename ProtoServiceType>
std::shared_ptr<ResourceRegistration> make_resource_registration() {
    // ...
    return std::make_shared<ResourceRegistration2<ClientType, ServerType, ProtoServiceType>>(sd);
}

Or perhaps it could be a member function of Registry:

class Registry {
public:
...
template<typename T, typename TClient, typename TServer>
void register_resource() { ... };

And then those init blocks would look like:

namespace {
bool init() {
    Registry::register_resource<MovementSensor, MovementSensorClient, MovementSensorServer>();
    return true;
};

// NOLINTNEXTLINE(cert-err58-cpp)
const bool inited = init();
}  // namespace

[acmorrow]

Suggest making this class final.

[acmorrow]

The MyProtoService type parameter could probably go away if all ResourceServer types had a required proto_service_type typedef. They all inherit from that type so it would make sense:

class MLModelServiceServer : public ResourceServer,
                             public ::viam::service::mlmodel::v1::MLModelService::Service {
   public:
       using proto_service_type = ::viam::service::mlmodel::v1::MLModelService::Service;
   ...
};

Then you could just say p->FindServiceByName(MyResourceServer::proto_service_type::service_full_name());, and then this template would have only two types: the Client and the Server (if you get rid of the CRTP).

Of course, that would require attaching that typedef to all server classes.

[acmorrow]

Also, @stuqdog, I know there are some plans afoot to eliminate the static initialization. There have also been some thoughts about making a cleaner separation between the client facing types and the server (e.g. module supporting) types. The current registration model requires that one part of the code (the registration type) know of both the Server and Client types, because there must be one ResourceRegistration subtype that can produce both types. That might be another thing to consider refactoring if/when the auto-registration is addressed.

[zaporter-work]

Thanks!

[zaporter-work]

Not hyped that this lives on the movement sensor object, but probably fine

[abe-winter]

want me to change to typedef?

Suggested change

	struct compassheading {
	/// A number from 0-359 where 0 is North, 90 is East, 180 is South, and 270 is West
	double value;
	};
	/// A number from 0-359 where 0 is North, 90 is East, 180 is South, and 270 is West
	typedef double compassheading;

[zaporter-work]

I don't have a strong opinion. typedef is probably better but either is fine. Feel free to resolve

[zaporter-work]

👍

[acmorrow]

LGTM with some small changes.

[acmorrow]

I only really looked at the resource registration changes, so my LGTM is limited to that. Overall, I think resource registration needs to be refactored and I expect that the final result will look more like a template member on Registry which does the work that ResourceRegistration2 does now.

[stuqdog]

Nice! A couple small comments/questions but otherwise looks great, thanks for doing this. No need to treat my re-review as blocking.

[benjirewis]

Potentially an unhelpful too-many-cooks situation if I review here, and my review is mostly qs 😆. I'll eventually follow up offline about those qs instead.