[RSDK-1648] Use rtkgps as a model #1742
Conversation
viambot
added
the
safe to test
| @@ -133,7 +133,7 @@ func init() { | |||
| cfg config.Component, | |||
| logger golog.Logger, | |||
| ) (interface{}, error) { | |||
| return newRTKStation(ctx, deps, cfg, logger) | |||
| return newRTKMovementSensor(ctx, deps, cfg, logger) | |||
There was a problem hiding this comment.
nice. We should add a test that catches this if it were still broken.
There was a problem hiding this comment.
I've added a guard, not totally convinced the pattern in base_test.go helps here, we're missing the concept of a movementsensor.Local. There is a way to rewrite it so that the creation function is based on a switch case depending on the resource name, but I'm not sure that's a good way to do it.
There was a problem hiding this comment.
Ugh you're right and I'm sorry I led you astray there. The test should be more like https://github.com/viamrobotics/rdk/blob/main/robot/session_test.go#L156 where we create a config, refer to the model, construct the robot, and get the component out of it. That's fairly end to end. You'll want to move this test into a new file with package gpsrtk_test so that you can import robot. Sorry!
There was a problem hiding this comment.
Lock the mutex when you write to your internal state, demonstrate that this works with actual GPS hardware, and things LGTM!
The real proof that this is right is when you can build a robot that uses this component, and have it give back your (at least approximate) location. Does that work?
I tried to do it during the bughunt, and got bogged down getting correction data. If it would help, I've got an account with the NYS Spatial Reference Network and would be happy to share my credentials (or you can make your own: it's free). but I never figured out how to plumb those credentials into the rest of the GPS (possibly because I was missing the current PR! 🙃 ), so don't have great advice on that.
| @@ -118,6 +118,7 @@ type cameramono struct { | |||
| logger golog.Logger | |||
| result result | |||
| stream gostream.VideoStream | |||
| mu sync.RWMutex | |||
There was a problem hiding this comment.
We lock the mutex whenever we read our state, but not when we write it. which means the mutex isn't yet preventing race conditions. Lock it in a few more places, too.
There was a problem hiding this comment.
Added, let me know if I've missed any.
There was a problem hiding this comment.
tl/dr: see the very last line.
Part of me says "any time anyone reads or writes any state within the struct (including result, stream, motion, and lastErr), they should lock the mutex." but that's overly broad:
streamis only used in the constructor (no chance of race conditions), the background goroutine, andClose(). and ifClose()causes trouble with the background thread, that's okay because we're closing things and it'll never be used again. So, it's probably fine that this isn't protected by the mutex.motionis only used by the background goroutine andextractMovementFromOdometer(), which is only called from the background goroutine. So, no chance of race conditions here, either. Part of me wonders if it can be made a local variable inbackgroundWorker()instead of a field in the struct, but that's probably out of scope for the current PR.- There will only be race conditions around
lastErrif copying and assigning it are not atomic. I don't know enough about Go's data model to know if that's a good assumption. For peace of mind, I'd protectlastErrwith the mutex just in case. - Every piece of
resultexceptresult.dtis protected by the mutex. However,result.dtappears to be written in one place and never read, so not protecting it seems fine. Maybe it can be removed entirely? but that's probably out of scope for the current PR.
So, I'd lock the mutex whenever reading or writing lastErr (and maybe even that is unnecessary), and the rest LGTM!
There was a problem hiding this comment.
results.dt is in the struct in case we ever need to mutate time for the algorithm long term, will lock if ever. And yes other things can be locked/unlocked without a defer but I feel unnecessary.
Will change lastErr mutex.
|
edit: no, you clarified this in the PR description, and I need to read better! 😅 |
viambot
added
safe to test
viambot
added
safe to test
This comment was marked as resolved.
This comment was marked as resolved.
Yes. The other model only cares about correction data, this model is a GPS floating around with the correction data resulting in coordinates with sub 2cm accuracy. The constructor function was copied wrong when I moved models around. |
viambot
added
safe to test
…48-get-the-gps-rtk-sensor-to-be-used-as-a-component
viambot
added
safe to test
| test.That(t, err, test.ShouldBeNil) | ||
|
|
||
| _, err = movementsensor.FromRobot(r, "station") | ||
| test.That(t, err.Error(), test.ShouldContainSubstring, gpsrtk.ErrStationValidation.Error()) |
There was a problem hiding this comment.
Checked that this breaks with the wrong creator type. Sufficient or should I spend more time on this and figure out how to mock the GPS, ntrip client, serial port, i2c port, and have this test consume them?
viambot
added
safe to test
…48-get-the-gps-rtk-sensor-to-be-used-as-a-component
viambot
added
safe to test
…48-get-the-gps-rtk-sensor-to-be-used-as-a-component
viambot
added
safe to test
viambot
added
safe to test
…48-get-the-gps-rtk-sensor-to-be-used-as-a-component
viambot
added
safe to test
|
randhid
deleted the
RSDK-1648-get-the-gps-rtk-sensor-to-be-used-as-a-component
branch
Mea culpa, when refactoring I used the wrong creator function for a model. Fixed.
Also added read locks for cameramono.