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

[WIP] Add functions for interacting with the primary battery configuration #73

Merged
merged 4 commits into from Jun 2, 2019

Conversation

@marmistrz
Copy link
Contributor

commented May 29, 2019

This pull request is intended to enable the control of the Primary Battery Charge Configuration from libsmbios, similar to what --PrimaryBattChargeCfg from cctk does.
These tokens are probably Dell-specific and will possibly allow advanced power management integration on Dell laptops using TLP. (Currently TLP only supports this feature on Thinkpads)

Things to be done before merging

  • deactivate the battery modes in set_charging_mode
  • connect the newly added functions to the CLI parser
  • validate the charge interval before writing it

I have encountered two problems while working on this PR:

  • how should I properly deactivate a token from the TokenTable? smbios_token.Token only exposes a method activate and no deactivate.
  • there are two available tokens for the custom charge (cf. class ChargingMode). Which of them should be used?
@superm1

This comment has been minimized.

Copy link
Member

commented May 29, 2019

how should I properly deactivate a token from the TokenTable? smbios_token.Token only exposes a method activate and no deactivate.

Typically is usually an opposite token for disable. You should "enable" that token to cause a disable.

there are two available tokens for the custom charge (cf. class ChargingMode). Which of them should be used?

You should use 0x343.

@superm1

This comment has been minimized.

Copy link
Member

commented May 29, 2019

(FYI, CI has a known failure with CentOS right now, haven't had time to debug it yet)

Show resolved Hide resolved src/bin/smbios-battery-ctl Outdated
@marmistrz

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2019

Typically is usually an opposite token for disable. You should "enable" that token to cause a disable.

Does it mean that, for instance, if I enable 0x0342, then 0x0343 will automatically get disabled?

@superm1

This comment has been minimized.

Copy link
Member

commented May 29, 2019

0x342 is setting "Adaptive Charge"
0x343 is setting "Custom Charge"

Only one can be set, so yes once you set 0x343 to be activated 0x342 will be deactivated.

@marmistrz marmistrz force-pushed the marmistrz:batt branch from cdb6a56 to 07b3386 May 31, 2019

@marmistrz

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2019

Let me know if you're ok with this CLI:

  --get-charging-cfg    Get the current Primary Battery Charge Configuration
  --set-charging-mode=<MODE>
                        Set the current Primary Battery Charge Configuration.
                        Valid choices are: ['primarily_ac', 'adaptive',
                        'custom', 'standard', 'express']
  --set-custom-charge-interval=<START> <END>
                        Set the percentage bounds for custom charge. Both
                        must be integers. START must lie in the range [50,
                        95], END must lie in the range [55, 100], END must be
                        at least (START + 5)
@superm1

This comment has been minimized.

Copy link
Member

commented May 31, 2019

Let me know if you're ok with this CLI:

I would prefer to see --set-custom-charge-interval split into two different CLI options that take an integer but otherwise it's fine.

if active:
if active_mode is not None:
print(f"\t WARNING: Inconsistency: both {active_mode} and\
{mode} enabled at the same time")

This comment has been minimized.

Copy link
@superm1

superm1 May 31, 2019

Member

Have you seen this happen? I would think this should be an Exception actually.

This comment has been minimized.

Copy link
@marmistrz

marmistrz May 31, 2019

Author Contributor

That's a good idea.

@marmistrz

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2019

I would prefer to see --set-custom-charge-interval split into two different CLI options that take an integer but otherwise it's fine.

Accepting the whole interval at once makes it much easier to validate its correctness.

@superm1

This comment has been minimized.

Copy link
Member

commented May 31, 2019

Accepting the whole interval at once makes it much easier to validate its correctness.

Can't you just do something like this:

if options.set_custom_charge_start and options.set_custom_charge_end:
    set_custom_charge_interval  ((start,end))

And you just do your validation in that routine?

@marmistrz

This comment has been minimized.

Copy link
Contributor Author

commented Jun 2, 2019

n't you just do something like this:

if options.set_custom_charge_start and options.set_custom_charge_end:
    set_custom_charge_interval  ((start,end))

And you just do your validation in that routine?

Then start & end have to be always supplied together, either none or both. Exposing them as separate options suggests otherwise. I know that the equality sign in the help is a little confusing, but I checked and argparse prints a much better help string (optparse is deprecated)

The PR should be ready.

@superm1 superm1 merged commit 0eab008 into dell:master Jun 2, 2019

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

marmistrz added a commit to marmistrz/libsmbios that referenced this pull request Jun 2, 2019

@marmistrz

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

When is the next release expected?

@superm1

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

End of the summer was the plan

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants
You can’t perform that action at this time.