RSDK-794 Sensor wrapper #174
Conversation
There was a problem hiding this comment.
Overall, this looks fine to me, but I have reservations about the current design ofserver_wrapper and client_stub and I don't want to see them go viral by duplication. I'll follow up with some suggestions about how I think they could be improved such that they could be factored out in this review. I missed my chance on the prior review but I didn't mind much because they were just in one place.
benjirewis
force-pushed
the
sensor-wrappers
branch
from
64c2918 to
4b9ed28
Compare
benjirewis
force-pushed
the
sensor-wrappers
branch
from
4b9ed28 to
092d732
Compare
There was a problem hiding this comment.
Used @acmorrow's ServiceHelper class for both SensorServer and MovementSensorServer. Removed custom ServerWrapperResult structs and server_wrapper methods.
|
|
||
| std::unordered_map<std::string, float> result = | ||
| status.movementsensor->get_accuracy(status.extra); | ||
| response->mutable_accuracy()->empty(); |
There was a problem hiding this comment.
I removed this line; I don't think it did anything (my guess is it was an attempt to "empty" the mutable_accuracy map, but empty just returns a boolean describing if the map is empty that we were ignoring).
| return ::grpc::Status(); | ||
| return make_service_helper<MovementSensor>("MovementSensorServer::GetAccuracy", this, request)( | ||
| [&](auto& helper, auto& movementsensor) { | ||
| const std::unordered_map<std::string, float> result = |
There was a problem hiding this comment.
This seems like a fine place for auto.
| return make_service_helper<MovementSensor>("MovementSensorServer::GetGeometries", | ||
| this, | ||
| request)([&](auto& helper, auto& movementsensor) { | ||
| const std::vector<GeometryConfig> geometries = |
| "SensorServer::GetReadings", this, request)([&](auto& helper, auto& sensor) { | ||
| const AttributeMap result = sensor->get_readings(helper.getExtra()); | ||
| for (const auto& r : *result) { | ||
| response->mutable_readings()->insert({std::move(r.first), r.second->proto_value()}); |
There was a problem hiding this comment.
r is const, so I suspect this move can't do anything.
|
|
||
| ::grpc::Status GetReadings(::grpc::ServerContext* context, | ||
| const GetReadingsRequest* request, | ||
| GetReadingsResponse* response) override; |
There was a problem hiding this comment.
These can and should now all be noexcept!
There was a problem hiding this comment.
Oops! Thank you; done.
RSDK-794
Implements wrappers for sensor API.
(cc @cheukt and @acmorrow).