Generic battery interface

Generic battery interface

[Pavel Machek]

Hi!

(Vojtech and other thinkpad users: look at tp_smapi package; it
includes stuff like "do not charge battery unless it is below 90%",
which should make your battery last few more years).

(I'll still need to test it on x60, sorry).

Anyway, unlike acpi, tp_smapi uses some reasonably-nice
one-value-per-file sysfs interface. I have few objections:

0) it needs to move to /sys/class/battery/tp_smapi or something like
that
 
1) it probably should not include units in the files (hard to parse,
and someone may have great idea to choose different units)

2) some optional method for battery applets to avoid polling would be
nice (OTOH tp_smapi probably does not support notifications, so it is
not its fault)

3) some fields look almost too specialized, but if hardware supports
them, I guess we should export them, too...

...but otherwise it looks very good, certainly better than
alternatives. Perhaps we should make something like this "generic"
battery interface?

Good description is at
http://thinkwiki.org/wiki/Tp_smapi#Installation_from_source , and to
give you a quick example:

# cat /sys/devices/platform/smapi/BAT0/installed
# cat /sys/devices/platform/smapi/BAT0/state       # idle/charging/discharging
# cat /sys/devices/platform/smapi/BAT0/cycle_count 
# cat /sys/devices/platform/smapi/BAT0/current_now # instantaneous  current
# cat /sys/devices/platform/smapi/BAT0/current_avg # last minute average
# cat /sys/devices/platform/smapi/BAT0/power_now   # instantaneous  power
# cat /sys/devices/platform/smapi/BAT0/power_avg   # last minute  average
# cat /sys/devices/platform/smapi/BAT0/last_full_capacity
# cat /sys/devices/platform/smapi/BAT0/remaining_capacity
# cat /sys/devices/platform/smapi/BAT0/design_capacity
# cat /sys/devices/platform/smapi/BAT0/voltage
# cat /sys/devices/platform/smapi/BAT0/design_voltage
# cat /sys/devices/platform/smapi/BAT0/manufacturer
# cat /sys/devices/platform/smapi/BAT0/model
# cat /sys/devices/platform/smapi/BAT0/barcoding
# cat /sys/devices/platform/smapi/BAT0/chemistry
# cat /sys/devices/platform/smapi/BAT0/serial
# cat /sys/devices/platform/smapi/BAT0/manufacture_date
# cat /sys/devices/platform/smapi/BAT0/first_use_date
# cat /sys/devices/platform/smapi/ac_connected

								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: Generic battery interface

[Matthew Garrett]

This would also be useful for the OLPC project - it's unlikely that 
it'll use ACPI, but a more feature-rich interface than /proc/apm would 
be massively helpful. This is just a matter of speccing out what 
information is needed and what format it should be presented in, and 
then adding a new device class, right?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: Generic battery interface

[Shem Multinymous]

On 7/27/06, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> This would also be useful for the OLPC project - it's unlikely that
> it'll use ACPI, but a more feature-rich interface than /proc/apm would
> be massively helpful. This is just a matter of speccing out what
> information is needed and what format it should be presented in, and
> then adding a new device class, right?

Can we really assume there's one driver providing all battery-related
attributes?

For example, on a ThinkPad you want the ACPI battery module loaded so
that handles hande battery-related ACPI events, but on ACPI can't
doesn't provide all available attributes. For example, ACPI reports
the equvialent of
  /sys/devices/platform/smapi/BAT0/power_avg (last minute average)
but not
  /sys/devices/platform/smapi/BAT0/power_now (instantaneous average)
or
  /sys/devices/platform/smapi/BAT0/cycle_count
or control functions like
  /sys/devices/platform/smapi/BAT0/force_discharge
(see http://thinkwiki.org/wiki/tp_smapi for detials).

In this particular case we could split the ACPI module into two, one
module for events and one module for the sysfs interface, and load
only the first one on ThinkPads. But that's only because tp_smapi
happens to reproduce all of ACPI's attributes; there are probably
other cases whether neither method dominates the other.

So, if we insist on a standard battery device class name, how do we
cope with multiple sources of information and control functions?

  Shem

Re: Generic battery interface

[Matthew Garrett]

On Thu, Jul 27, 2006 at 05:39:06PM +0300, Shem Multinymous wrote:

> Can we really assume there's one driver providing all battery-related
> attributes?

Hmm. That's a good point.

> So, if we insist on a standard battery device class name, how do we
> cope with multiple sources of information and control functions?

Ignoring the multiple sources of information bit for the moment, we need 
to figure out the correct method of event notification anyway. There's a 
long-term plan to get rid of /proc/acpi, so acpi notifications need to 
be more more generic in any case.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: Generic battery interface

[Greg KH]

On Thu, Jul 27, 2006 at 03:44:52PM +0100, Matthew Garrett wrote:
> On Thu, Jul 27, 2006 at 05:39:06PM +0300, Shem Multinymous wrote:
> 
> > Can we really assume there's one driver providing all battery-related
> > attributes?
> 
> Hmm. That's a good point.
> 
> > So, if we insist on a standard battery device class name, how do we
> > cope with multiple sources of information and control functions?
> 
> Ignoring the multiple sources of information bit for the moment, we need 
> to figure out the correct method of event notification anyway. There's a 
> long-term plan to get rid of /proc/acpi, so acpi notifications need to 
> be more more generic in any case.

You can poll sysfs files to be notified when states change...

thanks,

greg k-h

Re: Generic battery interface

[Pavel Machek]

Hi!

> >This would also be useful for the OLPC project - it's unlikely that
> >it'll use ACPI, but a more feature-rich interface than /proc/apm would
> >be massively helpful. This is just a matter of speccing out what
> >information is needed and what format it should be presented in, and
> >then adding a new device class, right?
> 
> Can we really assume there's one driver providing all battery-related
> attributes?

Anything else would be crazy, I'd say.

> For example, on a ThinkPad you want the ACPI battery module loaded so
> that handles hande battery-related ACPI events, but on ACPI can't
> doesn't provide all available attributes. For example, ACPI reports
> the equvialent of
>  /sys/devices/platform/smapi/BAT0/power_avg (last minute average)
> but not
>  /sys/devices/platform/smapi/BAT0/power_now (instantaneous average)
> or
>  /sys/devices/platform/smapi/BAT0/cycle_count
> or control functions like
>  /sys/devices/platform/smapi/BAT0/force_discharge
> (see http://thinkwiki.org/wiki/tp_smapi for detials).

Well, in that case probably smapi driver should "hook into" acpi
driver.

> In this particular case we could split the ACPI module into two, one
> module for events and one module for the sysfs interface, and load
> only the first one on ThinkPads. But that's only because tp_smapi
> happens to reproduce all of ACPI's attributes; there are probably
> other cases whether neither method dominates the other.

I hope such hardware will not be too common. Thinkpad is covered by
accident, and I do not know about any other problematic machine.

Worst case, we would get equivalent of

/sys/class/battery/system_battery_acpi/...
/sys/class/battery/system_battery_some_clever_vendor_hack/...

with some values common between both of them. I'd say it is still
better than having vendor_hack in /sys in one format while having acpi
battery in /proc/acpi in completely different format.
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: Generic battery interface

[Shem Multinymous]

On 7/27/06, Pavel Machek <pavel@suse.cz> wrote:
> Well, in that case probably smapi driver should "hook into" acpi
> driver.

That would be odd, the SMAPI driver has nothing to do with the ACPI
system. I don't think cross-system hooks are very popular...


> Worst case, we would get equivalent of
>
> /sys/class/battery/system_battery_acpi/...
> /sys/class/battery/system_battery_some_clever_vendor_hack/...
>
> with some values common between both of them. I'd say it is still
> better than having vendor_hack in /sys in one format while having acpi
> battery in /proc/acpi in completely different format.

Aye, we need for a consistent interface to common functionality.
How about /sys/class/battery/{acpi,apm,thinkpad}/BAT0?
So that for standard attributes, cat /sys/class/battery/*/BAT0/voltage
gives something reasonable.

  Shem

Re: [ltp] Re: Generic battery interface

[Henrique de Moraes Holschuh]

On Thu, 27 Jul 2006, Shem Multinymous wrote:
> On 7/27/06, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> >This would also be useful for the OLPC project - it's unlikely that
> >it'll use ACPI, but a more feature-rich interface than /proc/apm would
> >be massively helpful. This is just a matter of speccing out what
> >information is needed and what format it should be presented in, and
> >then adding a new device class, right?
> 
> Can we really assume there's one driver providing all battery-related
> attributes?

No, we cannot.  This is not true on ThinkPads, for example.  We *can* make
it true, but AFAIK that means overriding the entire generic ACPI battery
driver, and greating a "glue" thinkpad_battery one that does:

	1. generic ACPI battery access when ACPI is available
	2. thinkpad specific ACPI battery access when ACPI is available
	3. thinkpad specific EC firmware battery access (not ACPI).

That requires the new driver to use functions from ibm_acpi and tp_smapi,
plus the stuff in the generic acpi battery access driver.

It might be the best way to go for ThinkPads, actually.  Some rework on
ibm_acpi ain't that bad, and tp_smapi is already undergoing major surgery.

> In this particular case we could split the ACPI module into two, one
> module for events and one module for the sysfs interface, and load
> only the first one on ThinkPads. But that's only because tp_smapi
> happens to reproduce all of ACPI's attributes; there are probably
> other cases whether neither method dominates the other.

Yes.

> So, if we insist on a standard battery device class name, how do we
> cope with multiple sources of information and control functions?

Good question.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

RE: Generic battery interface

[Brown, Len]

>On 7/27/06, Brown, Len <len.brown@intel.com> wrote:
>> Path names and file names in sysfs are an API, so it is important
>> to choose them wisely.  The string "acpi" should not appear in
>> any path-name or file-name in sysfs that is intended to be generic,
>> as it would make no sense on a non-ACPI system.
>>
>> Neither the ACPI /proc/acpi/battery API or
>> the tp_smapi /sys/devices/platform/smapi API qualify as generic.
>> It it a historical artifact that /proc/acpi exists, I'd delete it
>> immediately if that wouldn't instantly break every distro on earth.
>
>Yes. But it looks like we'll have a hard time finding this common
>interface, due to overlaps and omissions between battery drivers.

Why not just specify the union of the different sets,
and those that don't implement parts leave those parts out?

>So I've proposed /sys/class/battery/{acpi,apm,thinkpad}/BAT?/* 
>as a comrpromise:
>A userspace app that only needs a generic voltage readout will try
>(all of) /sys/class/battery/*/BAT0/voltage. This is your generic
>interface.

If we have to export all these totally different implementations
via totally different paths, then we failed.

>A specialized app can be more specific and access
>/sys/class/battery/thinkpad/BAT0/force_discharge.
>
>And a complex system monitor like GKrellM will  let you configure a
>list of paths, e.g., /sys/class/battery/acpi/BAT0 +
>/sys/class/battery/mustek/UPS0.
>
>The only drawback I see is the inelegant userspace code for
>enumerating /sys/class/battery/* in languages like C. I suppose this
>could be wrapped by a trivial userspace library.

sysfs is great for demos from a shell prompt,
but isn't such a great programming interface, either
from inside the kernel or from user-space.

>> Vojtech suggested that we create a virtual battery interface,
>> just like the input layer has virtual input devices.
>[snip]
>> In either case, user-space would look for a well known set
>> of device file names, such as /dev/battery0, /dev/battery1
>> or some such, do a select on the file and get events that way.
>
>Isn't this an overkill? Battery status changes very slowly, so it
>seems perfectly adequate to have the usual userspace daemons poll the
>kernel every few seconds. No need for extra kernal event mechanisms,
>device file allocation, IOCTLs...

No need to poll for status that hasn't even changed.
This is what events are for.

Also, critical battery alarms are important events.

Please do not add more polling to user-space, else DaveJ
will be putting it up as a further example of "Why userspace sucks"
at the next OLS:-)

>> Drivers such as above could conjur up devices, and user-space
>> could also create what look like batteries, say through a
>> utility that knows how to talk to a UPS.
>
>Excellent idea, but it's orthogonal to the API issue. You might as
>well have  /sys/class/battery/usespace-battery/UPS0.

I think the question is where the GUI is going to go look for it.
I could imagine it would be simple for userspace to look for
/dev/battery0
/dev/battery1
/dev/battery2
until it finds no more, and then present the capacity of each
if it wants to.

-Len

Re: Generic battery interface

[Vojtech Pavlik]

On Thu, Jul 27, 2006 at 11:32:39PM +0300, Shem Multinymous wrote:

> >Also, critical battery alarms are important events.
> 
> Yes, but not time-sensitive.

They well can be. And it'd be pretty stupid to just throw away the
events the hardware offers to us and use polling from userspace, too.

> >Please do not add more polling to user-space, else DaveJ
> >will be putting it up as a further example of "Why userspace sucks"
> >at the next OLS:-)
> 
> Battery polling is already used extensively, and its overhead is
> completely negligible.

You're joking, right? On quite a number of laptops, it takes quite a
while to read the battery, spent in BIOS through SMI, polling the I2C
bus while talking to the battery. The less often this is done, the
better.

> I'm yet to see any deployed userspace code
> trying to query battery status more than once per second.

The applets that were doing it (yes, up to 100 times per second)
corrected their ways pretty quickly, because some machines became
unusable with the applet enabled.

> On the other hand, if you send an event whenever the voltage or
> current change, you'll flood userspace with junk. So you'll need to
> have a "fuzz" like in the input device code; but this may be
> client-specific or hardware-specific.

You could, trivially, mirror the behavior of current applets: Not report
the changes to the battery status more often than each N seconds, except
for critical events.

Said that, there haven't been many problems reported in the input layer
that I could attibute to bad default selection of fuzz.

> And you'll need to identify devices in a useful way, a problem that's
> not yet solved even for input devices... You see where it's going.

May you be more specific here? I'm not aware of any problems in this
area. This may be my fault: What needs to be fixed there?

-- 
Vojtech Pavlik
Director SuSE Labs

Re: Generic battery interface

[Shem Multinymous]

Hi,

On 7/28/06, Vojtech Pavlik <vojtech@suse.cz> wrote:
> > Battery polling is already used extensively, and its overhead is
> > completely negligible.
>
> You're joking, right? On quite a number of laptops, it takes quite a
> while to read the battery, spent in BIOS through SMI, polling the I2C
> bus while talking to the battery. The less often this is done, the
> better.

Yes, I know -- tp_smapi does that too. And it's still negligible,
usually a few microseconds.

Heck, the hdaps driver polls that same I2C bus 50 times per seconds
and still doesn't tickle the load average.


> > I'm yet to see any deployed userspace code
> > trying to query battery status more than once per second.
>
> The applets that were doing it (yes, up to 100 times per second)
> corrected their ways pretty quickly, because some machines became
> unusable with the applet enabled.

Exactly -- and they've been working merrily ever since.
And if you don't want to trust applet developers, cache the latest
reads and refresh them only if X jiffies have passed.


> You could, trivially, mirror the behavior of current applets: Not report
> the changes to the battery status more often than each N seconds, except
> for critical events.

You're taking a polling-based hardware, exposing it as an event-based
interface, and then and kludging it so that it behaves like polling
again...

So, in this scheme, how many lines of code does is the equivalent of
"cat /sys/devices/platform/smapi/BAT0/voltage"?


> > And you'll need to identify devices in a useful way, a problem that's
> > not yet solved even for input devices... You see where it's going.
>
> May you be more specific here? I'm not aware of any problems in this
> area. This may be my fault: What needs to be fixed there?

"Generic interface for accelerometers (AMS, HDAPS, ...)" on LKML, a
few weeks ago, about moving accelerator-based hard disk parking from
sysfs polling to the the input infrastructure. One unresolved issue
was how to find which input device happens to be the relevant
accelerometer.

  Shem

Re: Generic battery interface

[Vojtech Pavlik]

On Fri, Jul 28, 2006 at 03:27:00AM +0300, Shem Multinymous wrote:

> >You're joking, right? On quite a number of laptops, it takes quite a
> >while to read the battery, spent in BIOS through SMI, polling the I2C
> >bus while talking to the battery. The less often this is done, the
> >better.
> 
> Yes, I know -- tp_smapi does that too. And it's still negligible,
> usually a few microseconds.

The load isn't the problem. The incurred latencies - both interrupt and
scheduling - are. Audio playback skips, mice losing sync, keyboards
losing keystrokes, these are the nasty effects I've seen so far.

> Heck, the hdaps driver polls that same I2C bus 50 times per seconds
> and still doesn't tickle the load average.

The Analog Devices ADXL2xx sensors in the HDAPS are not implementing
I2C, only having analog and PWM outputs. I doubt they're connected over
I2C to the EC.

> >The applets that were doing it (yes, up to 100 times per second)
> >corrected their ways pretty quickly, because some machines became
> >unusable with the applet enabled.
> 
> Exactly -- and they've been working merrily ever since.
> And if you don't want to trust applet developers, cache the latest
> reads and refresh them only if X jiffies have passed.

The timer interrupt still has to happen every time their select() or
sleep() expires, with the system having to wake up, even when nothing
happened. Polling from userspace is bad.

> >You could, trivially, mirror the behavior of current applets: Not report
> >the changes to the battery status more often than each N seconds, except
> >for critical events.
> 
> You're taking a polling-based hardware, exposing it as an event-based
> interface, and then and kludging it so that it behaves like polling
> again...

Every (I2C, direct ADC and more) sensor is polling-based by nature. The
eventization can happen in the EC, the BIOS, or later in the chain -
kernel or userspace. The earlier you stop it, the better for your power
consumption.

On event-based interface, the program using it doesn't have to use
events (it still can read the immediate values explicitly), on a
polling-based interface nobody can use events.

The event-based interface can even signal a certain device will not
supply any events and needs to be polled. This would the interface to
match the hardware better at the expense of making it more complex.

> So, in this scheme, how many lines of code does is the equivalent of
> "cat /sys/devices/platform/smapi/BAT0/voltage"?

NOTE: I'm arguing event-based vs poll-based here. This is orthogonal to
the /dev vs /sys - both can supply or not supply events.

It's two lines in C, if you omit error checking.

	fd = open("/dev/bat0", O_RDONLY);
	ioctl(fd, BATCGVOLTAGE, &voltage);

for the sysfs implementation (for comparison), you'll need (at minimum):

	fd = open("/dev/bat0", O_RDONLY);
	read(fd, buf, MAX_LEN);
	voltage = strtol(buf, buf + MAX_LEN, 10);

If you want a shell script, you'd use a small utility supplied with the
reference implementation:

	batstate -t voltage /dev/bat0

And it'd of course give you the same output as the 'cat' line.

> >> And you'll need to identify devices in a useful way, a problem that's
> >> not yet solved even for input devices... You see where it's going.
> >
> >May you be more specific here? I'm not aware of any problems in this
> >area. This may be my fault: What needs to be fixed there?
> 
> "Generic interface for accelerometers (AMS, HDAPS, ...)" on LKML, a
> few weeks ago, about moving accelerator-based hard disk parking from
> sysfs polling to the the input infrastructure. One unresolved issue
> was how to find which input device happens to be the relevant
> accelerometer.

The current well known methods are:

	1) udev/hotplug. It can create device nodes and symlinks based on the
		capabilities and IDs of an input device.
	1a) HAL. It has all the info from hotplug as well.
	2) open them all and do the capability checks / IDs yourself.
	3) (obsolete, deprecated) parse /proc/bus/input/devices, which
		lists all the input devices

Any problems with that?

-- 
Vojtech Pavlik
Director SuSE Labs

Re: Generic battery interface

[Shem Multinymous]

On 7/28/06, Vojtech Pavlik <vojtech@suse.cz> wrote:
> On Fri, Jul 28, 2006 at 03:27:00AM +0300, Shem Multinymous wrote:

> > Yes, I know -- tp_smapi does that too. And it's still negligible,
> > usually a few microseconds.
>
> The load isn't the problem. The incurred latencies - both interrupt and
> scheduling - are. Audio playback skips, mice losing sync, keyboards
> losing keystrokes, these are the nasty effects I've seen so far.

ACK. But I still don't what difference it makes, in this respect, if
apps poll the kernel which queries the hardware (maybe with caching)
vs. kernel queries the kernel and informs apps.


> The Analog Devices ADXL2xx sensors in the HDAPS are not implementing
> I2C, only having analog and PWM outputs. I doubt they're connected over
> I2C to the EC.

The ADXL320 accelerometer is sampled by the H8S/2161B embedded
controller, and the host polls the controller over LPC.


> > Exactly -- and they've been working merrily ever since.
> > And if you don't want to trust applet developers, cache the latest
> > reads and refresh them only if X jiffies have passed.
>
> The timer interrupt still has to happen every time their select() or
> sleep() expires, with the system having to wake up, even when nothing
> happened.

Yes, it becomes an issue with tickless. Tell them now to poll more
often then they need, then...

Here's the thing. An event mechanism makes sense when discrete events
happen at irregular interval determined by the data source. But here,
you're trying to convert a (pontentially) continuous function like
voltage to a sporadic stream of "the value changed a lot" events. So
you need to "fuzz" the stream like in the input layer, but you have no
idea what the applications need. One client may try to monitor power
fluctuations with 10Hz updates, while another may log once per minute.

The exception is "status goes critical"-type events. For sysfs, the
attribute change uevent (mentioned by Greg KH) looks like a perfect
solution for these.


> NOTE: I'm arguing event-based vs poll-based here. This is orthogonal to
> the /dev vs /sys - both can supply or not supply events.

>         fd = open("/dev/bat0", O_RDONLY);
>         ioctl(fd, BATCGVOLTAGE, &voltage);

So you *are* proposing polling fo rthe /dev interface too? I thought
the main argument for the /dev

OK, I agree the issues are orthogonal (once sysfs attr change uevents
are added, anyway). My take:
1. We need polling. We

> for the sysfs implementation (for comparison), you'll need (at minimum):
>
>         fd = open("/dev/bat0", O_RDONLY);
>         read(fd, buf, MAX_LEN);
>         voltage = strtol(buf, buf + MAX_LEN, 10);

No, you can use fscanf.


> If you want a shell script, you'd use a small utility supplied with the
> reference implementation:
>
>         batstate -t voltage /dev/bat0
>
> And it'd of course give you the same output as the 'cat' line.

And then we have to maintain both a kernel side and a userspace side.
And what do I, poor author of tp_smapi, do if I want to add a
non-standard attribute? Tell people to patch and overwrite their
disto's batstate binary too?


> The current well known methods are:
>
>         1) udev/hotplug. It can create device nodes and symlinks based on the
>                 capabilities and IDs of an input device.
>         1a) HAL. It has all the info from hotplug as well.
>         2) open them all and do the capability checks / IDs yourself.

Ooh, that's messy. I'll have to think about it.


Meanwhile, here's another issue with the accelerometer input device
(and by analogy, with batteries): client-specific event rate and fuzz.
Neverball only needs "a big change has happened" events, maybe 10-20
times per second. The disk parking daemon needs a perfectly accurate
readouts at 50Hz or better, plus it needs to know whenever a sample is
taken (even if it didn't change since the previous sample). How can
this be handled without multiple input devices?

  Shem

Re: Generic battery interface

[Vojtech Pavlik]

On Fri, Jul 28, 2006 at 06:14:57PM +0300, Shem Multinymous wrote:

> >The load isn't the problem. The incurred latencies - both interrupt and
> >scheduling - are. Audio playback skips, mice losing sync, keyboards
> >losing keystrokes, these are the nasty effects I've seen so far.
> 
> ACK. But I still don't what difference it makes, in this respect, if
> apps poll the kernel which queries the hardware (maybe with caching)
> vs. kernel queries the kernel and informs apps.

Not that much. The difference then remains only on a tickless system.

> >The Analog Devices ADXL2xx sensors in the HDAPS are not implementing
> >I2C, only having analog and PWM outputs. I doubt they're connected over
> >I2C to the EC.
> 
> The ADXL320 accelerometer is sampled by the H8S/2161B embedded
> controller, and the host polls the controller over LPC.

No I2C. LPC is so much faster than I2C (8 MHz 8-bit vs 100 kHz serial).

> >The timer interrupt still has to happen every time their select() or
> >sleep() expires, with the system having to wake up, even when nothing
> >happened.
> 
> Yes, it becomes an issue with tickless. Tell them now to poll more
> often then they need, then...
> 
> Here's the thing. An event mechanism makes sense when discrete events
> happen at irregular interval determined by the data source. But here,
> you're trying to convert a (pontentially) continuous function like
> voltage to a sporadic stream of "the value changed a lot" events. So
> you need to "fuzz" the stream like in the input layer, but you have no
> idea what the applications need. One client may try to monitor power
> fluctuations with 10Hz updates, while another may log once per minute.


> The exception is "status goes critical"-type events. For sysfs, the
> attribute change uevent (mentioned by Greg KH) looks like a perfect
> solution for these.
> 
> 
> >NOTE: I'm arguing event-based vs poll-based here. This is orthogonal to
> >the /dev vs /sys - both can supply or not supply events.
> 
> >        fd = open("/dev/bat0", O_RDONLY);
> >        ioctl(fd, BATCGVOLTAGE, &voltage);
> 
> So you *are* proposing polling fo rthe /dev interface too? I thought
> the main argument for the /dev

I'm not proposing polling. I'm proposing that there be a way how to read
the immediate values, in addition to the event notifications. 

Yes, this means polling would be possible.

Reading immediate values is a must, if only to know the values at the
start of your monitoring applet.

> OK, I agree the issues are orthogonal (once sysfs attr change uevents
> are added, anyway). My take:
> 1. We need polling. We
> 
> >for the sysfs implementation (for comparison), you'll need (at minimum):
> >
> >        fd = open("/dev/bat0", O_RDONLY);
> >        read(fd, buf, MAX_LEN);
> >        voltage = strtol(buf, buf + MAX_LEN, 10);
> 
> No, you can use fscanf.

Correct.

> >If you want a shell script, you'd use a small utility supplied with the
> >reference implementation:
> >
> >        batstate -t voltage /dev/bat0
> >
> >And it'd of course give you the same output as the 'cat' line.
> 
> And then we have to maintain both a kernel side and a userspace side.
> And what do I, poor author of tp_smapi, do if I want to add a
> non-standard attribute? Tell people to patch and overwrite their
> disto's batstate binary too?

How often do you plan to do that? Anyway, the answer is yes, it's not a
big deal to do.

> >The current well known methods are:
> >
> >        1) udev/hotplug. It can create device nodes and symlinks based on 
> >        the
> >                capabilities and IDs of an input device.
> >        1a) HAL. It has all the info from hotplug as well.
> >        2) open them all and do the capability checks / IDs yourself.
> 
> Ooh, that's messy. I'll have to think about it.

As Dmitry pointed out, all the info (except for the events) is in sysfs,
too.

> Meanwhile, here's another issue with the accelerometer input device
> (and by analogy, with batteries): client-specific event rate and fuzz.
> Neverball only needs "a big change has happened" events, maybe 10-20
> times per second. The disk parking daemon needs a perfectly accurate
> readouts at 50Hz or better, plus it needs to know whenever a sample is
> taken (even if it didn't change since the previous sample). How can
> this be handled without multiple input devices?

You need at least 50 Hz for reasonable game control, too. I remember
that analog joysticks sampled at 10 Hz were unusable.

The input layer was designed for input devices that control applications
by actions of the user. The fuzz filtering was designed with that in
mind and is expected to be set once at boot either by an educated guess
of a kernel driver or by the system administrator. Because it's designed
for input devices, it has these properties:

	* It ignores minor noise
	* It slowly reacts to continuous drift of the values
	* It reacts immediately to large changes

This would be likely completely unsuitable for batteries and may be bad
for a drive parking daemon, too. If the daemon can't use it, it'll need
another interface. 

-- 
Vojtech Pavlik
Director SuSE Labs

Re: Generic battery interface

[Shem Multinymous]

On 7/28/06, Vojtech Pavlik <vojtech@suse.cz> wrote:

> Not that much. The difference then remains only on a tickless system.

And even then, only when apps use excessive poll rates (way above
1Hz), in which case the apps are broken; and broken apps can eat power
in ways much worse than msleep(10) on a tickless system.


> No I2C. LPC is so much faster than I2C (8 MHz 8-bit vs 100 kHz serial).

Ack.


> > >NOTE: I'm arguing event-based vs poll-based here. This is orthogonal to
> > >the /dev vs /sys - both can supply or not supply events.
> >
> > >        fd = open("/dev/bat0", O_RDONLY);
> > >        ioctl(fd, BATCGVOLTAGE, &voltage);
> >
> > So you *are* proposing polling fo rthe /dev interface too? I thought
> > the main argument for the /dev
>
> I'm not proposing polling. I'm proposing that there be a way how to read
> the immediate values, in addition to the event notifications.
>
> Yes, this means polling would be possible.

So your /dev proposal is equivalent, functionality-wise, to sysfs +
attribute change uevent, right? The only question is if we do it
through /dev+ioctls or /sysfs+fscanf.


> > And then we have to maintain both a kernel side and a userspace side.
> > And what do I, poor author of tp_smapi, do if I want to add a
> > non-standard attribute? Tell people to patch and overwrite their
> > disto's batstate binary too?
>
> How often do you plan to do that?

With tp_smapi, I did it about 10 times over half a year. And there's
probably more to come.


> Anyway, the answer is yes, it's not a big deal to do.

Practically, it's much messier. As a developer/tester you have two
components to juggle instead of one, and it's a mess when they get out
of sync.


> As Dmitry pointed out, all the info (except for the events) is in sysfs,
> too.

And as I pointed out in reply, matching up devices in /sys and /dev is
extremely cumbersome (and prone to race conditions).


> > Meanwhile, here's another issue with the accelerometer input device
> > (and by analogy, with batteries): client-specific event rate and fuzz.
> > Neverball only needs "a big change has happened" events, maybe 10-20
> > times per second. The disk parking daemon needs a perfectly accurate
> > readouts at 50Hz or better, plus it needs to know whenever a sample is
> > taken (even if it didn't change since the previous sample). How can
> > this be handled without multiple input devices?
>
> You need at least 50 Hz for reasonable game control, too. I remember
> that analog joysticks sampled at 10 Hz were unusable.

If you rattle your ThinkPad this badly, latency in Neverball is going
to be the least of your problems. :-)

The mainline hdaps driver does 20Hz, BTW.


> The input layer was designed for input devices that control applications
> by actions of the user. The fuzz filtering was designed with that in
> mind and is expected to be set once at boot either by an educated guess
> of a kernel driver or by the system administrator. Because it's designed
> for input devices, it has these properties:
>
>         * It ignores minor noise
>         * It slowly reacts to continuous drift of the values
>         * It reacts immediately to large changes
>
> This would be likely completely unsuitable for batteries and may be bad
> for a drive parking daemon, too. If the daemon can't use it, it'll need
> another interface.

There's a pattern here. Maybe what we need is a generic scheme for
publishing continuous readouts, that will work for both hdaps and
batteries (despite a X100 difference in typical time magnitudes).
Doubtless there are other uses for such a scheme. We want it to
support clients with different desired rates, data sources with
different update frequencies, use minimum CPU and avoid unnecesary
timer interrupts on tickless kernels.

Here's one approach: use a syscall (e.g., ioctl) saying "block until
there's new data on this fd, or N milliseconds have passed, whichever
is *later*". This way each client declares the update rate it wants
and can change it on the fly. The driver sees all the requests and can
perform the minimum hardware quering -- for example, it won't query
the hardware at all if no client has submitted a request with
parameter N more than N milliseconds go. And there's no excessive work
or interrupts. Some (simple) kernel code infrastructure is needed to
help drivers manage the pending requests.

Extending this approach, we can have a call for "block until the
readout has changed by M, or N milliseconds have passed, whichever is
later". This may not reduce query rate for polling-based hardware, but
will reduce the event rate.

I suppose these calls should also have a (normal) timeout, for the
usual reasons.

  Shem

Re: Generic battery interface

[Shem Multinymous]

On 7/29/06, Shem Multinymous <multinymous@gmail.com> wrote:

> There's a pattern here. Maybe what we need is a generic scheme for
> publishing continuous readouts, that will work for both hdaps and
> batteries (despite a X100 difference in typical time magnitudes).
> Doubtless there are other uses for such a scheme. We want it to
> support clients with different desired rates, data sources with
> different update frequencies, use minimum CPU and avoid unnecesary
> timer interrupts on tickless kernels.
>
> Here's one approach: use a syscall (e.g., ioctl) saying "block until
> there's new data on this fd, or N milliseconds have passed, whichever
> is *later*". This way each client declares the update rate it wants
> and can change it on the fly. The driver sees all the requests and can
> perform the minimum hardware quering -- for example, it won't query
> the hardware at all if no client has submitted a request with
> parameter N more than N milliseconds go. And there's no excessive work
> or interrupts. Some (simple) kernel code infrastructure is needed to
> help drivers manage the pending requests.

Here's a rough sketch for the userspace side of a continuous function
sampling interface. It handles the blocking a bit better than the
above proposal, in that it lets you easily handle multiple readouts.
It's agnostic about /dev vs. /sys.

As always, we have a file handing out readouts obtained from the
hardware - either a device file or a sysfs attribute. The file has the
following properties:

1. A new ioctl, SYSFS_DELAYED_UPDATE, meaning: "I want an a fresh
   readout in N milliseconds, or whenver it's ready, whichever is later.
   If I poll this FD with POLLIN, wait until the new readout is ready.
   Likewise for select."
2. When the file is opened in O_NONBLOCKing mode, read() always return
   the latest cached readout rather than querying the hardware
   (unless the latter has negligible cost).
3. When the file is opened in normal (blocking) mode, read() blocks
   until a fresh readout is available and returns this readout; see
   below.

To illustrte, here's an example of a proper polling loop (sans error
checking). This app wants updates at most every second, but obviously
not more frequently than the hardware has to offer. If no readout ca
be obtained for 20 seconds, it warns the user that the data is stale.

--------------------------
/* Open attribute file with O_NONBLOCK so that all reads will
 * return cached values instead of blocking:
 */
int fd = open("/whatever/voltage", O_NONBLOCK|O_RDONLY);

/* Read and process latest cached attribute value: */
read(fd, ...);
...

while (1) {
	struct pollfd ufds = { .fd=fd, events=POLLIN };

	/* Tell driver we want a fresh readout but no sooner than 1sec
	 * from now:
	 */
	ioctl(fd, SYSFS_DELAYED_UPDATE, 1000);

	/* Wait for an update for at most 5 seconds. Nominally this will
	 * block for at least 1 second, because of the above ioct. If we
	 * were reading multiple attributes, we could poll them all
	 * simultaneously.
	 */
	poll(&ufds, 1, 20000);
	... warn user if timed out ...

	/* Read and process latest cached attribute value: */
	read(fd, ...);
	... handle readout ...
}
--------------------------

Note that the above does not ensure minimal separation between
readouts:  you're guaranteed a fresh readout at every iteration, but
it might have been taken at the beginning of the >1sec poll period. If
you need to ensure that all readouts are at least (700ms apart, add a
sleep(700) at the end of the loop and decrease 1000 to 300 in the
ioctl().I think most apps won't need this.

Legacy and lightweight applications (e.g., cat from shell) will open
the file in blocking mode. In this case, a "read(fd, ...)" has
semantics equivalent to the following O_NONBLOCK-mode code:

--------------------------
	/* Tell driver we want an immediate update: */
	ioctl(fd, ATTR_DELAY_UPDATE, 0);

	/* Wait indefinitely for an update: */
	poll(&ufds, 1, -1);

	/* Read the latest cached attribute value: */
	read(fd, ...);
--------------------------

Similarly, select() and poll() on non-blocking files acts like you did
"ioctl(fd, ATTR_DELAY_UPDATE, 0);" and then wait for a refresh or timeout.

If we want to penalize people not using nonblocking mode and delayed
updates, we can replace two "0" above with a sufficiently nasty
(driver-dependent?) constant.

How does this look?

I'm not getting into the kernel side for now; it's doable, and with
proper infrastructure (e.g., at the sysfs level) can be elegant and
efficient.

  Shem

Re: Generic battery interface

[Shem Multinymous]

Hi all,

Here's an updated rough proto-spec for the userspace side of the
continuous-parameter polling interface, intended for hwmon, batteries
and their ilk, and maybe even the input infrastructure.

Compared to the parent post, this version adds a second parameter to
the ioctl (see discussion elsewhere in this thread), and corrects a
few inaccuracies.

With an event-based data source, this interface allows polling with no
time interrupts on tickless kernels. With a polling-based data source
and a bit of luck, the frequency of polling = frequency of induced
timer interrupts will be the minimum that satisfies the greediest
client (even if there are many). In general, complicated data sources
can be handled optimally by custom driver+infrastructure code. All of
this is completely transparent  to userspace, which just states its
needs and has its every desire fulfilled.

Hardware readouts are obtained from a dedicated file - a sysfs
attribute (as in hwmon and tp_smapi) or a device file (as in the input
infrastructure). The file has the following properties:

1. A new ioctl DELAYED_UPDATE, with parameters min_wait and
   min_fresh, meaning: "I want an a fresh readout. If I poll() this FD
   with POLLIN then send an input-ready event at time is T+min_wait, or
   when you have a readout that was received from the hardware at  time
   T+min_fresh, whichever is *later*. Likewise if I select()".
   Here T is the time of the ioctl call and min_wait>=min_fresh.
2. When the file is opened in O_NONBLOCKing mode, read() always return
   the latest cached readout rather than querying the hardware
   (unless the latter has negligible cost).
3. When the file is opened in normal (blocking) mode, read() blocks
   until a fresh readout is available and returns this readout; see
   below.

To illustrate, here's an example of a proper polling loop (sans error
checking). This app wants to refresh its display when the data has
changed, but not more often than once per second. It wants the
readouts to be reasonly spaced: they should be obtained at least 700ms
apart. And it needs to update its GUI every 3 seconds regardless of
readouts.

In some linux/foo.h:
--------------------------
struct delayed_update_req {
	int min_wait;
	int min_fresh;
};
--------------------------

Application code:
--------------------------
/* Open attribute file with O_NONBLOCK so that all reads will
* return cached values instead of blocking:
*/
int fd = open("/whatever/voltage", O_NONBLOCK|O_RDONLY);

/* Read and process latest cached attribute value: */
read(fd, ...);
...

while (1) {
	const struct delayed_update_req dureq =
		{ .min_wait=1000, min_fresh=700 };
	struct pollfd ufds = { .fd=fd, events=POLLIN };

	/* Tell the driver we want a fresh readout, but no sooner than
	 * 1sec from now, and we want the readout to reflect reality no
	 * sooner than 700ms now:
	 */
	ioctl(fd, DELAYED_UPDATE, &dureq);

	/* Wait for an update for at most 3 seconds. Nominally this will
	* block for at least 1 second, because of the above ioct. If we
	* were reading multiple attributes, we could poll them all
	* simultaneously.
	*/
	poll(&ufds, 1, 3000);

	/* Read latest cached attribute value: */
	read(fd, ...);

	... handle readout, update GUI ...
}
--------------------------

Legacy and lightweight applications (e.g., cat from shell) will open
the file in blocking mode. In this case, a "read(fd, ...)" has
semantics equivalent to the following O_NONBLOCK-mode code:

--------------------------
	const struct delayed_update_req dureq =
		{ .min_wait=0, min_fresh=0 };

	/* Tell driver we want an immediate update: */
	ioctl(fd, DELAYED_UPDATE, &dureq);

	/* Wait indefinitely for an update: */
	poll(&ufds, 1, -1);

	/* Read the latest cached attribute value: */
	read(fd, ...);
--------------------------

Similarly, select() and poll() on non-blocking files acts like you did
"ioctl(fd, DELAYED_UPDATE, &dureq)" with dureq={0,0} and then waited
for a refresh or timeout.

If we want to penalize code which doesn't use nonblocking mode and
delayed updates, we can increase the implicit dureq.min_wait above
from 0 to a sufficiently nasty (driver-dependent?) delay.


Comments?

(I still can't implement this myself. Just trying to elucidate my suggestion.)

  Shem

Re: [ltp] Re: Generic battery interface

[Shem Multinymous]

On 7/31/06, Michael Olbrich <michael.olbrich@gmx.net> wrote:
> On Sun, Jul 30, 2006 at 09:37:23PM +0300, Shem Multinymous wrote:
> >
> > Comments?
>
> Hmmm, that looks good for most cases. But how would you handle
> starting/stoping laoding/draining the battery?
> That usually results in values jumping from/to 0. A gui would want to
> show such a change immediately while otherwise keeping a slow update
> rate. Maybe some kind of threshhold parameter? "send an input-ready
> immediatelly if the value changes by more than x%".

Changes by more than x% compared to what?

The value at the time of the ioctl()? This might completely miss a
change that happened between the previous read() and the ioctl().

The value at the time of the last read()? Then the kernel
driver+infrastructure will need to keep track of the latest readout
done by each app. That's pretty heavy.

So what semantics make sense?

  Shem

Re: [ltp] Re: Generic battery interface

[Shem Multinymous]

Hi Michael,

Please do a Reply to All. I've restored the mailing list CCs.

On 7/31/06, Michael Olbrich <michael.olbrich@gmx.net> wrote:
> On Mon, Jul 31, 2006 at 06:18:12PM +0300, Shem Multinymous wrote:
> > On 7/31/06, Michael Olbrich <michael.olbrich@gmx.net> wrote:
> > >Hmmm, that looks good for most cases. But how would you handle
> > >starting/stoping laoding/draining the battery?
> > >That usually results in values jumping from/to 0. A gui would want to
> > >show such a change immediately while otherwise keeping a slow update
> > >rate. Maybe some kind of threshhold parameter? "send an input-ready
> > >immediatelly if the value changes by more than x%".
> >
> > Changes by more than x% compared to what?
> >
> > The value at the time of the ioctl()? This might completely miss a
> > change that happened between the previous read() and the ioctl().
> >
> > The value at the time of the last read()? Then the kernel
> > driver+infrastructure will need to keep track of the latest readout
> > done by each app. That's pretty heavy.
> >
> > So what semantics make sense?
>
> Well, the kernel is polling the hardware and caches the last value for
> O_NONBLOCKing anyway. So we already have two values to compare.

It's caching *one* value, not one value per polling app  (i.e., opened file).
Also, if querying the hardware is very cheap, the driver is at freedom
to do that instead of caching.


> And keeping the latest readout for each app isn't that heavy. After all
> we already have to keep track of the timeouts for each app.

The timeouts bookkeeping will normally be done by some infrastructure,
and can often be (in principle) be optimized to less than on value per
app. Also, it's just one timestamp. By contrast, what you're asking
for requires explicit handling by every driver, and the attribute
value may take significant amount of storage -- per app.


> Another way to handle this:
> A lot of applications are interrested in changes. It would be perfectly
> reasonable for a poll() to block for over 1 minute while the value
> fluctuates with changes less than e.g. 1%. As soon as the change
> (compared to the last readout) exceds this threshhold poll() returns
> immediatelly. This could of course be combined with the proposed
> timeouts.

The app can do this itself by polling and checking the value, with a
(not too) small value of dupeq.min_wait. In the case of a
polling-based data source, the resulting hardware queries and timer
interrupts are exactly the same as an in-kernel implementation which
does the polling and comparions itself. If the data source is
event-based then the comparison in userspace does have a drawback: the
comparions are done just dupeq.min_wait apart even if the event rate
happens to be higher. Can you think of a case where this matters?

  Shem

Re: [ltp] Re: Generic battery interface

[Shem Multinymous]

Hi Micael,

I ask again: please do a Reply to All. This discussion is CCed on
several mailing lists, not just link-thinkpad.

On 8/1/06, Michael Olbrich <michael.olbrich@gmx.net> wrote:
> On Tue, Aug 01, 2006 at 01:45:27AM +0300, Shem Multinymous wrote:
> >>And keeping the latest readout for each app isn't that heavy. After all
> >>we already have to keep track of the timeouts for each app.
> >
> >The timeouts bookkeeping will normally be done by some infrastructure,
> >and can often be (in principle) be optimized to less than on value per
> >app. Also, it's just one timestamp. By contrast, what you're asking
> >for requires explicit handling by every driver, and the attribute
> >value may take significant amount of storage -- per app.
>
> If you are that concerned about storage why the complex timeout model?
> That can easily handled in userspace with just the blocking and
> nonblocking reads.

Please explain how this can be done  in a way that (a) works
transparently with both event-driven and query-based drivers, (b)
handles multiple clients efficiently, (c) minimizes hardware queries
in the case of query-based drivers, and (d) doesn't cause unnecessary
timer interrupts on tickless kernels.


> > The app can do this itself by polling and checking the value, with a
> > (not too) small value of dupeq.min_wait. In the case of a
> > polling-based data source, the resulting hardware queries and timer
> > interrupts are exactly the same as an in-kernel implementation which
> > does the polling and comparions itself. If the data source is
> > event-based then the comparison in userspace does have a drawback: the
> > comparions are done just dupeq.min_wait apart even if the event rate
> > happens to be higher. Can you think of a case where this matters?
>
> The problem I see is the overhead. Visual feedback that feels
> instantaneous would require dupeq.min_wait<50ms. And as far as I can
> tell each read requires to switch from userspace to kernelspace and
> back. When I look at the available variables I can easily imagine
> applications that would read >10 variables. That's not something I would
> want to do that often.

This is relevant only to query-based drivers, not event-based. How
expensive is a context switch compared to a typical hwmon hardware
query?

  Shem