Skip to content
New issue

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

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

Already on GitHub? # to your account

Default to no EPR/PPS so its opt in #2073

Merged
merged 1 commit into from
Feb 8, 2025
Merged

Default to no EPR/PPS so its opt in #2073

merged 1 commit into from
Feb 8, 2025

Conversation

Ralim
Copy link
Owner

@Ralim Ralim commented Feb 8, 2025

Some chargers have funky PPS.
Plus some do not want EPR be default (even though 28V is safe by designer on PinecilV2).

So take the safest option by default and make it opt-in

  • Please check if the PR fulfills these requirements
  • The changes have been tested locally
  • There are no breaking changes
  • What kind of change does this PR introduce?

Request from Pinecil Chat - d​simic

  • What is the current behavior?
  • What is the new behavior (if this is a feature change)?

  • Other information:

Some chargers have funky PPS.
Plus some do not want EPR be default (even though 28V is safe by designer on PinecilV2).

So take the safest option by default and make it opt-in
@ia ia self-requested a review February 8, 2025 06:05
@ia ia enabled auto-merge (squash) February 8, 2025 06:06
Copy link
Collaborator

@ia ia left a comment

Choose a reason for hiding this comment

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

Just my own humble two cents...

Probably it will be worth mentioning somewhere in the docs, so I will try not to forget to address this in the future.

I understand the risk having this on by default, but one of the main features (not available with any other supported hardware) is full EPR/28V mode support, but having this off by default will probably raise a lot of questions from freshly new users who is getting Pinecil V2 for this reason only. :/

@@ -93,7 +93,7 @@ static const SettingConstants settingsConstants[(int)SettingsOptions::SettingsOp
{ 0, 6, 1, 1}, // LOGOTime
{ 0, 1, 1, 0}, // CalibrateCJC
{ 0, 1, 1, 0}, // BluetoothLE
{ 0, 2, 1, 1}, // USBPDMode
{ 0, 2, 1, 0}, // USBPDMode
Copy link
Collaborator

Choose a reason for hiding this comment

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

Interesting... Definitely looks more safer, especially when users connect PD-compatible supported hardware to not-so-reliable PD-kinda power sources.

@ia ia merged commit ebdff59 into dev Feb 8, 2025
36 checks passed
@ia ia deleted the default-no-epr-pps branch February 8, 2025 06:17
@Ralim
Copy link
Owner Author

Ralim commented Feb 8, 2025

Yeah it's a trade off, between safest and what people expect.
I don't know which is better, but safest is probably wise for now I guess. And it's just a default so easier to toggle.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants