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

low upload / default upload / full upload toggle for cellular #30500

Closed
wants to merge 8 commits into from

Conversation

yuzisee
Copy link

@yuzisee yuzisee commented Nov 20, 2023

Description

Attempts to implement a toggle on cell:

▸ full upload -- i have unlimited, upload whatever (we will grab your segments over cell, not available for full prime)
▸ prime upload -- current prime policy (default)
▸ low upload -- prime policy without qlog/qcam

Original discussion https://discord.com/channels/469524606043160576/954493346250887168/1161103504761434182

Verification

I'm completely new to the openpilot codebase, so any pointers on how I can test this would be super helpful!

I used #24742 as a reference for how we make three-choice toggles, and used #23734 as a reference for how the upload policy is implemented.

Thanks!

@wired4sound
Copy link

wired4sound commented Nov 27, 2023

Would like to see something along the lines of an option that when you request a file upload via connect, it doesnt have to wait for wifi. Or a "force" option in the connect uploads interface to tell comma not to wait for wifi on that particular file?

Use case:

I get to work. I want to look at some log files. I request upload of said logs via Connect. Doh! Must wait until after work to dive into my logs since no wifi and I can't go out to my car to hotspot it during work.

@jakethesnake420
Copy link
Contributor

jakethesnake420 commented Nov 27, 2023

Seems like a no brainer to have this feature but as it stands, this code is not up to par to be merged.

It's probably better to modify the dispatcher with a bool param to allow uploads over cellular when requested explicitly by the user.

allow_cellular=file.allow_cellular,

However, comma connect might complain that uploads are paused on cellular but it should still work.

yuzisee added a commit to yuzisee/openpilot-dev that referenced this pull request Nov 28, 2023
"It's probably better to modify the dispatcher with a bool param to allow uploads over cellular when requested explicitly by the user."
@yuzisee
Copy link
Author

yuzisee commented Nov 28, 2023

this code is not up to par to be merged

Yeah, happy to make improvements!

modify the dispatcher with a bool param to allow uploads over cellular when requested explicitly by the user

Cool, added 74a2859 just now; is it close to what you had in mind?

"It's probably better to modify the dispatcher with a bool param to allow uploads over cellular when requested explicitly by the user."
@jakethesnake420
Copy link
Contributor

jakethesnake420 commented Nov 28, 2023

Keep it simple, this is stock openpilot. All you need to do is use the GsmMetered param in athenad and add a toggle to stop uploader when on a metered connection. Then update the tests. Oh and try testing your code and you will see it doesn't stop q uploads

yuzisee added a commit to yuzisee/openpilot-dev that referenced this pull request Dec 1, 2023
…two options, instead of three.

▸ "low upload" mode (works the same as pull request commaai#30500)
▸ "default upload" mode (works the same as pull request commaai#30500)
▸ "full upload" mode: not implemented here

Without a "full upload" option, we can skip checking to see whether we are using a Comma SIM (i.e. whether the user is subscribed to full prime?)
@yuzisee
Copy link
Author

yuzisee commented Dec 1, 2023

Keep it simple, this is stock openpilot. All you need to do is use the GsmMetered param in athenad and add a toggle to stop uploader when on a metered connection.

Thanks. I put a simplified version here #30569 that doesn't implement the entire original description but is much simpler and follows the approach you suggest. It's lighterweight and easier to work with for sure 🙂

Then update the tests. Oh and try testing your code and you will see it doesn't stop q uploads

Yes! As I mentioned above #30500 (comment) I'm completely new to the openpilot codebase, so any pointers on how I can test this would be super helpful!

Copy link
Contributor

github-actions bot commented Jan 1, 2024

This PR has had no activity for 30 days. It will be automatically closed in 7 days if there is no activity.

@github-actions github-actions bot added the stale label Jan 1, 2024
Copy link
Contributor

github-actions bot commented Jan 9, 2024

This PR has been automatically closed due to inactivity. Feel free to re-open once activity resumes.

@github-actions github-actions bot closed this Jan 9, 2024
@adeebshihadeh adeebshihadeh reopened this Jan 9, 2024
@github-actions github-actions bot removed the stale label Jan 10, 2024
Copy link
Contributor

This PR has had no activity for 30 days. It will be automatically closed in 7 days if there is no activity.

@github-actions github-actions bot added the stale label Feb 10, 2024
Copy link
Contributor

This PR has been automatically closed due to inactivity. Feel free to re-open once activity resumes.

@github-actions github-actions bot closed this Feb 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants