Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ramp up / ramp down #281

Closed
wants to merge 57 commits into from
Closed

Conversation

lmeier
Copy link
Contributor

@lmeier lmeier commented Oct 30, 2023

No description provided.

@lmeier lmeier force-pushed the liammeier/fence-1461-sdk-ramp-up-ramp-down branch from 958a35e to c92cde7 Compare November 6, 2023 18:12
@lmeier lmeier force-pushed the liammeier/fence-1461-sdk-ramp-up-ramp-down branch 2 times, most recently from 953ec1f to 0c6243f Compare November 21, 2023 18:17
@lmeier lmeier changed the title [WIP] Ramp up / ramp down Ramp up / ramp down Dec 7, 2023
@lmeier lmeier marked this pull request as ready for review December 7, 2023 18:32
@lmeier lmeier force-pushed the liammeier/fence-1461-sdk-ramp-up-ramp-down branch from 16a24e1 to 922c9c3 Compare December 7, 2023 19:05
return options;
}

+ (RadarTrackingOptions *)rampedUpOptions {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is continuous not good enough? If not, wondering if we reuse it but change 1-2 of the options via code instead of creating a new preset. If we did want to create a new preset we would probably want to name it presetXxx and also think of a more descriptive name then "ramped up".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

think essentially we want continuous but with more frequent updates / faster sync interval.

IMO the intended tracking options are what works for microdetection / DD use case

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. RampingOption and StateChange seem to add alot of complexity that will be hard to get right I think. Two things I'm thinking about:

  1. Would it help if Radar.startTripWithOptions() accepted a second trackingOptions param that if set kicks in when within the radius? And we would expand the "previous tracking options" concept from 1 to a list, maybe.
  2. Would it help if a single bundle of tracking options had a few more fields representing the ramped up needs? It might only be the update intervals and sync intervals. It would be less generic bc you couldn't change anything else when ramping, but the code to handle those values would be alot simpler I suspect.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've taken a stab at number 2, cutting a waypoint build now: e68d2f5

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tactical thing, I think the force pushes are making it hard to see what changed since I looked at this last. Everything looks like it was pushed 3 days ago. I'll fiddle around in github, see if I can find a way. I guess lacking that, what changed to implement #2?

But more generally, the idea behind my comment was to get rid of RampingOption and StateChange because I think they add too much complexity. I'll spend the day tomorrow trying to understand how those work and try to form a more detailed opinion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yikes, that makes sense. Didn't mean to be force pushing. Will watch out for that.

What I did was remove the notion of there being any difference between "ramped up" tracking options and "normal / ramped down" tracking options. Instead, the tracking options are fixed — eliminating some of the state complexity you pointed out, not having to worry about keeping track of the tracking options over time.

Still, we need to keep track of past ramped state so that we can timeout of being ramped up. This is how we address the problem of being persistently (or for a long time) close to a ramp up geofence.

We might also choose to ramp down when stopped: true (GK suggested this today).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look forward to hearing the more detailed opinion that you form

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. AreRampingOption and StateChange primarily needed to be able to timeout? Or are they important for other reasons? Ramping down or shutting down when stopped is an interesting idea.

How long is the timeout?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Timeout is 20min in 1 hour OR 120 minutes in 12 hours

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, correct that StateChange is for the timeout.

The RampingOption, perhaps deserving of a more fitting name, is just the enum passed to updateTracking to indicate whether updateTracking should ramp up, ramp down, or no-op.

@lmeier lmeier force-pushed the liammeier/fence-1461-sdk-ramp-up-ramp-down branch from e765c47 to fee3e94 Compare January 2, 2024 18:07
Comment on lines +98 to +99
_lowPowerLocationManager.desiredAccuracy = kCLLocationAccuracyHundredMeters;
_lowPowerLocationManager.distanceFilter = kCLDistanceFilterNone;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lowPowerLocationManager changes a la #303

@lmeier
Copy link
Contributor Author

lmeier commented Mar 1, 2024

Closing as we're opting to do this server side

@lmeier lmeier closed this Mar 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants