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

General bugfixes #253

Merged
merged 9 commits into from
Jun 28, 2024
Merged

General bugfixes #253

merged 9 commits into from
Jun 28, 2024

Conversation

thdrmrphy
Copy link
Contributor

@thdrmrphy thdrmrphy commented Apr 5, 2024

This PR:

  • adds validation, based on @ksgubanov's work on Add validation for the percentage values #228, but slightly fixed
  • fixes eternal loops where disable_discharging would disable_charging, even when called from within enable_charging
  • fixes some MagSafe light stuff
  • adds more verbose logging - this can probably be fixed; it's quite messy, but it's more descriptive than before
  • takes accuracy code from Improve accuracy when stopping charging #241 and seems to work properly ¯\_(ツ)_/¯
  • still has issues, but doesn't add any new issues that I know of.

I'm very much open to suggestions. This PR kind of throws everything at trying to fix bugs and so may have made some things a bit messier than they used to be. Having said that, I've been stress-testing this code daily for a while and I've not run into the original issues any longer.

Hope it helps!

@thdrmrphy thdrmrphy changed the title Bugfixes General bugfixes Apr 5, 2024
@thdrmrphy thdrmrphy marked this pull request as ready for review April 5, 2024 06:59
@jkellerer
Copy link

@thdrmrphy thanks a lot for the efforts in improving the app. However the change in #244 broke one of the main features of the app, the ability to disable the charge limit and charge the battery to 100% (from the UI). Because when you disable it, "stop" is the $setting and it ends up in the disable_charge branch (... what also looks strange is that battery_percentage is only updated after use but that doesn't change anything for that matter).

This PR is likely fixing this part, but I'm not convinced that disable_discharge should do anything else than setting the state with smc (and be symmetric to enable_discharge), especially when it is used at many locations that expect its original behaviour.

I'd consider a new method if the same sort of logic (disabling discharge and maintaining the state) is needed at multiple locations.

What should not happen, however is that the primary function of the app (limit the charge and allow to toggle the limit from the UI) breaks.

@Chr1s70ph
Copy link
Contributor

Is there anything I can contribute to this MR?
I would greatly appreciate the fixes and even tried fixing them myself before i found this

@thdrmrphy
Copy link
Contributor Author

Hey @jkellerer thanks for taking a look at this. It's been a bit since I really took a look at this code so forgive me if I'm missing something. I do think this PR fixes that issue (I'm not convinced the issue was caused by #244 but I don't think I helped it by adding the extra logic) and from what I remember, disable_discharging had to do all that new logic because if one disabled discharging by unticking the option called "Allow force-discharging" (which also had some Electron issues all along but that's a different matter) then the app would have ignored the last-used setting. As far as I'm concerned, the app should do everything it does and then have a layer on top that enables/disables force-discharging, which otherwise doesn't affect the app's function.

Also, I have no idea why I put battery_percentage=$(get_battery_percentage) at the end. No clue. If someone can work out what should be happening that would be awesome, I assume it should just be at the start but it's been a while and I don't remember.

Having said all that there's a good chance I'm missing something and whilst I haven't had the issue you're describing with the UI ("it works on my machine") the app currently needs a lot of reworking and this PR is definitely not ready to merge. @Chr1s70ph thank you so much for the help, to anyone wanting to contribute I think the next steps that we can take here are probably to identify and document the issues that @jkellerer mentioned (as well as any others that pop up) including the relevant logs from the app and then we can try to find out where it's going wrong. Also, error handling (or lack thereof) and variable validation are major issues here so if perhaps we can try to implement checks at every possible point (so that we don't keep getting issues where the app is trying to work out whether a string or a numerical value is higher). From memory there might be some other open PRs to help with this.

I'll keep an eye on this and try to contribute where I can but thanks for the input everyone and I hope this gets us somewhere more stable.

@Chr1s70ph Chr1s70ph mentioned this pull request Apr 21, 2024
6 tasks
@ksgubanov
Copy link

closes #210

@actuallymentor actuallymentor merged commit a316b55 into actuallymentor:main Jun 28, 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.

5 participants