Bug 124223
| Summary: | [acpi] [patch] acpi_battery.c -- Notify user-defined critical level via devd(8) | ||||||
|---|---|---|---|---|---|---|---|
| Product: | Base System | Reporter: | Pietro Cerutti <gahr> | ||||
| Component: | kern | Assignee: | freebsd-acpi (Nobody) <acpi> | ||||
| Status: | Closed FIXED | ||||||
| Severity: | Affects Only Me | ||||||
| Priority: | Normal | ||||||
| Version: | 8.0-CURRENT | ||||||
| Hardware: | Any | ||||||
| OS: | Any | ||||||
| Attachments: |
|
||||||
Responsible Changed From-To: freebsd-bugs->freebsd-acpi Over to maintainer(s). -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA512 POKE! Anybody interested in reviewing it? - -- Pietro Cerutti gahr@FreeBSD.org PGP Public Key: http://gahr.ch/pgp -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.9 (FreeBSD) iEYEAREKAAYFAkjBQI8ACgkQwMJqmJVx946CrQCfY7I1sTPwoPte89cP5zXg5j8S YNUAnjO41f8uzOPFDRR9XAvcEiHcB9ID =ef6A -----END PGP SIGNATURE----- There are a few problems with your approach. Critical status is already reported with a flag when usermode polls for the battery status: > if (sc->bst.state & ACPI_BATT_STAT_CRITICAL) { > if ((sc->flags & ACPI_BATT_STAT_CRITICAL) == 0) { > sc->flags |= ACPI_BATT_STAT_CRITICAL; > device_printf(dev, "critically low charge!\n"); > } > } Since usermode utilities already poll, they can handle that flag or implement their own notion of critical battery level. Why introduce a new kernel thread to do that same polling? Don't common battery status tools that poll (say, xbatt) have their own way to set a critical level? -Nate Pietro Cerutti wrote: > POKE! > > Anybody interested in reviewing it? -- Nate -----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
Nate Lawson wrote:
| There are a few problems with your approach.
|
| Critical status is already reported with a flag when usermode polls for
| the battery status:
|> if (sc->bst.state & ACPI_BATT_STAT_CRITICAL) {
|> if ((sc->flags & ACPI_BATT_STAT_CRITICAL) == 0) {
|> sc->flags |= ACPI_BATT_STAT_CRITICAL;
|> device_printf(dev, "critically low charge!\n");
|> }
|> }
I agree. Critical level is already checked for in the cmbat module.
However, reporting is not done in a "standardized" way. My patch would
just like to add notification through devd.
| Since usermode utilities already poll, they can handle that flag or
| implement their own notion of critical battery level. Why introduce a
| new kernel thread to do that same polling?
Because you can't configure the notion of "critical level" via cmbat.
|
| Don't common battery status tools that poll (say, xbatt) have their own
| way to set a critical level?
xbatt uses APM and ioctl to get the status of the battery.
I agree that userlevel utilities can e.g., retrieve the remaining
percent and set the critical level arbitrarily, but I anyway think that
a configurable level to be seen as critical by the OS could be a useful
addition.
I'm open to discussions about the best way to implement it (cmbat,
battery, ...?), if we get to an agreement that this could be a useful
feature :) Otherwise feel free to close the PR, as I can accept your
arguments.
|
| -Nate
|
| Pietro Cerutti wrote:
|> POKE!
|>
|> Anybody interested in reviewing it?
|
- --
Pietro Cerutti
gahr@FreeBSD.org
PGP Public Key:
http://gahr.ch/pgp
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.9 (FreeBSD)
iEYEAREKAAYFAkjFnJcACgkQwMJqmJVx946mtgCgivQVqG6nrctsffXFdm5HRQfX
1OUAoL9TXYwwiNfHvnUclKAhVzWcnw9S
=3q5J
-----END PGP SIGNATURE-----
Pietro Cerutti wrote: > Nate Lawson wrote: > | There are a few problems with your approach. > | > | Critical status is already reported with a flag when usermode polls for > | the battery status: > |> if (sc->bst.state & ACPI_BATT_STAT_CRITICAL) { > |> if ((sc->flags & ACPI_BATT_STAT_CRITICAL) == 0) { > |> sc->flags |= ACPI_BATT_STAT_CRITICAL; > |> device_printf(dev, "critically low charge!\n"); > |> } > |> } > > I agree. Critical level is already checked for in the cmbat module. > However, reporting is not done in a "standardized" way. My patch would > just like to add notification through devd. But it doesn't just add notification through devd. It adds a thread, that separately polls for battery state and sends a notify through devd. If the user is running any battery app, there's a double poll for the same info. I subscribe to the design approach that where it makes sense to do something in usermode, don't do it in kernel mode. In this case, the IO interface is poll-only, and any user app that is running can set its own policy for how to deal with the information it gets from polling. > I agree that userlevel utilities can e.g., retrieve the remaining > percent and set the critical level arbitrarily, but I anyway think that > a configurable level to be seen as critical by the OS could be a useful > addition. > > I'm open to discussions about the best way to implement it (cmbat, > battery, ...?), if we get to an agreement that this could be a useful > feature :) Otherwise feel free to close the PR, as I can accept your > arguments. I think it would be better to work with the third-party apps to support configurable levels. In xbatt: /* If battery life is short, ring a bell */ if ((life != APM_STAT_UNKNOWN) && (life < 5) && ((life % 5) == 0)) { ... and ... /* set gage color */ if (displayType == Color) { if (life < 3) { gage_color = "red"; } else if (life < 5) { gage_color = "yellow"; } } These thresholds could easily be a command-line var or X resource. Let's keep this in usermode, where policy belongs. -- Nate -----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
Nate Lawson wrote:
| Pietro Cerutti wrote:
|> Nate Lawson wrote:
|> | There are a few problems with your approach.
|> |
|> | Critical status is already reported with a flag when usermode polls for
|> | the battery status:
|> |> if (sc->bst.state & ACPI_BATT_STAT_CRITICAL) {
|> |> if ((sc->flags & ACPI_BATT_STAT_CRITICAL) == 0) {
|> |> sc->flags |= ACPI_BATT_STAT_CRITICAL;
|> |> device_printf(dev, "critically low charge!\n");
|> |> }
|> |> }
|>
|> I agree. Critical level is already checked for in the cmbat module.
|> However, reporting is not done in a "standardized" way. My patch would
|> just like to add notification through devd.
|
| But it doesn't just add notification through devd. It adds a thread,
| that separately polls for battery state and sends a notify through devd.
| If the user is running any battery app, there's a double poll for the
| same info.
|
| I subscribe to the design approach that where it makes sense to do
| something in usermode, don't do it in kernel mode. In this case, the IO
| interface is poll-only, and any user app that is running can set its own
| policy for how to deal with the information it gets from polling.
[snip xbatt-related stuff]
| Let's keep this in usermode, where policy belongs.
That's fine. Thanks for reviewing that!
Best,
- --
Pietro Cerutti
gahr@FreeBSD.org
PGP Public Key:
http://gahr.ch/pgp
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.9 (FreeBSD)
iEYEAREKAAYFAkjFo7UACgkQwMJqmJVx945TJQCfTRuG0ZiMSfyIaw0rb/5C1cXY
E4oAoJdERo/AA7KwGRtYnVEQeUoPo9Az
=UAwz
-----END PGP SIGNATURE-----
State Changed From-To: open->closed Policy belongs to usermode. |
Critically low battery levels are notified by the ACPI subsystem via the acpi_cmbat.c module. This prints a line on the console. The problem with cmbat is that the "critically low level" value is not configurable. The following patch implements a kernel process within acpi_battery.c. Two sysctl OIDs are exported in order to control the polling rate and the life % to be considered critical. When this critical level is reached, acpi_battery.c notifies userland via devd(8), allowing for a devd.conf(5) configuration such as: notify 10 { match "system" "ACPI"; match "subsystem" "Battery"; match "notify" "0x80"; action "logger -p kern.emerg 'WARNING: Low battery!'"; };