RE: Generic battery interface

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

[Shem Multinymous]

On 7/27/06, Brown, Len <len.brown@intel.com> wrote:
> Why not just specify the union of the different sets,
> and those that don't implement parts leave those parts out?

That's cool, as long as we still let drivers express minor differences
within the framework (see my last message to Daniel).


> >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.

They're not totally different, they differ in a very specific way
which allows the difference to be ignored by applications that don't
care.


> 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.

<shrug>
I have my own list of sysfs gripes (e.g., how do you implement a
"query a register" interface?), but the powers that be decreed we're
supposed to use sysfs for this kind of stuff.


> Also, critical battery alarms are important events.

Yes, but not time-sensitive.


> 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. I'm yet to see any deployed userspace code
trying to query battery status more than once per second.

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. 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.

  Shem

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

[Matthew Garrett]

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.

With ACPI "smart" batteries, we're currently spending long enough on 
querying batteries that people are losing keystrokes. The same is true 
of several crummy APM implementations. On the other hand, I agree that 
it makes sense to leave polling in userspace rather than the kernel. 
Once we go tickless on portable hardware, there's going to be a pretty 
huge incentive to fix userspace in any case.

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

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

[Pavel Machek]

Hi!

> > >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.

I do not understand this. Any polling (in kernel or in userspace) will
wake the CPU, wasting power.

OTOH "high/low/very low" battery applet can reasonably query battery
every 5 minutes, while detailed, graphical thingie displaying the
current power consumption will probably poll every 10 seconds...
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: Generic battery interface

[Vojtech Pavlik]

On Fri, Jul 28, 2006 at 02:25:08PM +0200, Pavel Machek wrote:
> Hi!
> 
> > > >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.
> 
> I do not understand this. Any polling (in kernel or in userspace) will
> wake the CPU, wasting power.

The kernel, however, has all the gory details at hand, and can decide
much better about the polling frequency, than the (hopefully) hardware
agnostic userspace.

Imagine your Zaurus: You don't need to poll very often when you are on
the flat part of the LiIon discharge curve, you probably want more
detailed info near the end.

> OTOH "high/low/very low" battery applet can reasonably query battery
> every 5 minutes, while detailed, graphical thingie displaying the
> current power consumption will probably poll every 10 seconds...

-- 
Vojtech Pavlik
Director SuSE Labs

Re: Generic battery interface

[Shem Multinymous]

On 7/28/06, Vojtech Pavlik <vojtech@suse.cz> wrote:
> > I do not understand this. Any polling (in kernel or in userspace) will
> > wake the CPU, wasting power.
>
> The kernel, however, has all the gory details at hand, and can decide
> much better about the polling frequency, than the (hopefully) hardware
> agnostic userspace.
>
> Imagine your Zaurus: You don't need to poll very often when you are on
> the flat part of the LiIon discharge curve, you probably want more
> detailed info near the end.

Another example: on my ThinkPad, the readouts provided by the EC
change only every 2 seconds, regardless of how much you poll it.

Hence my proposal for a polling-based interface with kernel-side
caching. This way, the hardware query rate is the minimum of two
rates: what the application can use and what the kernel thinks is the
hardware's change rate.

This doesn't solve the timer interrupt (tickless kernel) issue. Sane
apps would never ask for battery updates at more than a few Hz, are we
likely to see tickless machines where this makes a difference
beyondnormal load?

  Shem

Re: Generic battery interface

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 552 bytes --]

On Fri, 28 Jul 2006 18:38:13 +0300, Shem Multinymous said:
>
> Another example: on my ThinkPad, the readouts provided by the EC
> change only every 2 seconds, regardless of how much you poll it.
> 
> Hence my proposal for a polling-based interface with kernel-side
> caching. This way, the hardware query rate is the minimum of two
> rates: what the application can use and what the kernel thinks is the
> hardware's change rate.

Is there a reliable (or hack-worthy) way for the kernel to determine how
often the values are re-posted by the hardware?

[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: Generic battery interface

[Shem Multinymous]

On 7/28/06, Valdis.Kletnieks@vt.edu <Valdis.Kletnieks@vt.edu> wrote:
> Is there a reliable (or hack-worthy) way for the kernel to determine how
> often the values are re-posted by the hardware?

That's hardware-specific. Some drivers can know, others may just
assume 1sec or 0.1sec or whatever.

  Shem

Re: Generic battery interface

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 634 bytes --]

On Sat, 29 Jul 2006 01:10:40 +0300, Shem Multinymous said:
> On 7/28/06, Valdis.Kletnieks@vt.edu <Valdis.Kletnieks@vt.edu> wrote:
> > Is there a reliable (or hack-worthy) way for the kernel to determine how
> > often the values are re-posted by the hardware?
> 
> That's hardware-specific. Some drivers can know, others may just
> assume 1sec or 0.1sec or whatever.

That smells suspiciously like "We need an API for the hardware-specific
bits f code to pass the generic bits a value for this..." (and the
hardware-specific part can either ask the battery, or return a
hard-coded "10 seconds" that somebody measured, or whatever)....

[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: Generic battery interface

[Shem Multinymous]

On 7/29/06, Valdis.Kletnieks@vt.edu <Valdis.Kletnieks@vt.edu> wrote:
> On Sat, 29 Jul 2006 01:10:40 +0300, Shem Multinymous said:
> > On 7/28/06, Valdis.Kletnieks@vt.edu <Valdis.Kletnieks@vt.edu> wrote:
> > > Is there a reliable (or hack-worthy) way for the kernel to determine how
> > > often the values are re-posted by the hardware?
> >
> > That's hardware-specific. Some drivers can know, others may just
> > assume 1sec or 0.1sec or whatever.
>
> That smells suspiciously like "We need an API for the hardware-specific
> bits f code to pass the generic bits a value for this..." (and the
> hardware-specific part can either ask the battery, or return a
> hard-coded "10 seconds" that somebody measured, or whatever)....

I don't think "update frequency" is a good abstraction. The hardware's
update may not be variable and irrregular (e.g., event-based), and
there's there's an issue of phase sync to avoid unnecessary latency.

The lazy polling approach I described in my last post to Vojtech
("block until there's  a new readout or N milliseconds have passed,
whichever is later") looks like a more general, accurate and efficient
interface.

  Shem

Re: Generic battery interface

[Vojtech Pavlik]

On Sat, Jul 29, 2006 at 12:48:51PM +0300, Shem Multinymous wrote:

> I don't think "update frequency" is a good abstraction. The hardware's
> update may not be variable and irrregular (e.g., event-based), and
> there's there's an issue of phase sync to avoid unnecessary latency.
> 
> The lazy polling approach I described in my last post to Vojtech
> ("block until there's  a new readout or N milliseconds have passed,
> whichever is later") looks like a more general, accurate and efficient
> interface.
 
If "N" is given by the kernel, then it's identical to an event-based
approach. ;) Just described in different words.

-- 
Vojtech Pavlik
Director SuSE Labs

Re: Generic battery interface

[Shem Multinymous]

On 7/29/06, Vojtech Pavlik <vojtech@suse.cz> wrote:
> > The lazy polling approach I described in my last post to Vojtech
> > ("block until there's  a new readout or N milliseconds have passed,
> > whichever is later") looks like a more general, accurate and efficient
> > interface.
>
> If "N" is given by the kernel, then it's identical to an event-based
> approach. ;) Just described in different words.

No, N is given separately by each userspace client, on every call.
That's the whole point. The kernel driver then does the minimal
hardware querying and event generation that (a) makes sense for the
hardware and (b) satisfies all userspace clients.

  Shem

Re: Generic battery interface

[Mark Underwood]

--- Shem Multinymous <multinymous@gmail.com> wrote:

> On 7/29/06, Valdis.Kletnieks@vt.edu <Valdis.Kletnieks@vt.edu> wrote:
> > On Sat, 29 Jul 2006 01:10:40 +0300, Shem Multinymous said:
> > > On 7/28/06, Valdis.Kletnieks@vt.edu <Valdis.Kletnieks@vt.edu> wrote:
> > > > Is there a reliable (or hack-worthy) way for the kernel to determine how
> > > > often the values are re-posted by the hardware?
> > >
> > > That's hardware-specific. Some drivers can know, others may just
> > > assume 1sec or 0.1sec or whatever.
> >
> > That smells suspiciously like "We need an API for the hardware-specific
> > bits f code to pass the generic bits a value for this..." (and the
> > hardware-specific part can either ask the battery, or return a
> > hard-coded "10 seconds" that somebody measured, or whatever)....
> 
> I don't think "update frequency" is a good abstraction. The hardware's
> update may not be variable and irrregular (e.g., event-based), and
> there's there's an issue of phase sync to avoid unnecessary latency.
> 
> The lazy polling approach I described in my last post to Vojtech
> ("block until there's  a new readout or N milliseconds have passed,
> whichever is later") looks like a more general, accurate and efficient
> interface.
> 

This sounds like a good idea. You could do a similar thing using sysfs by
providing a entry in sysfs which tells userland when the next update is going
to happen, the userland app can then decide to use this as it's next poll time
or not.

Mark

>   Shem
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 



		
___________________________________________________________ 
Yahoo! Photos – NEW, now offering a quality print service from just 7p a photo http://uk.photos.yahoo.com

Re: Generic battery interface

[Shem Multinymous]

On 7/29/06, Mark Underwood <basicmark@yahoo.com> wrote:
> > The lazy polling approach I described in my last post to Vojtech
> > ("block until there's  a new readout or N milliseconds have passed,
> > whichever is later") looks like a more general, accurate and efficient
> > interface.
>
> This sounds like a good idea. You could do a similar thing using sysfs by
> providing a entry in sysfs which tells userland when the next update is going
> to happen, the userland app can then decide to use this as it's next poll time
> or not.

The driver may not know when the next update will happen, e.g., if its
data source is event-based. Moreover, the driver may be able to
*decide* when the next update will happen (i.e., when to query
poll-based hardware) if the clients say what they need in advance. The
"lazy poll" / "reverse select" handles both of these.

  Shem

Re: Generic battery interface

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 669 bytes --]

On Sat, 29 Jul 2006 12:48:51 +0300, Shem Multinymous said:

> The lazy polling approach I described in my last post to Vojtech
> ("block until there's  a new readout or N milliseconds have passed,
> whichever is later") looks like a more general, accurate and efficient
> interface.

That's not good.

If the program says '100ms' because it knows it will need to do a GUI update
then, and you block it for 5 seconds because that's when the next value
update happens, the user is stuck looking at their gkrellm or whatever not
doing anything at all for 4.9 seconds....

This almost forces the use of multiple threads if the program wants to do
its own timer management.

[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: Generic battery interface

[Vojtech Pavlik]

On Sun, Jul 30, 2006 at 04:35:57AM -0400, Valdis.Kletnieks@vt.edu wrote:
> On Sat, 29 Jul 2006 12:48:51 +0300, Shem Multinymous said:
> 
> > The lazy polling approach I described in my last post to Vojtech
> > ("block until there's  a new readout or N milliseconds have passed,
> > whichever is later") looks like a more general, accurate and efficient
> > interface.
> 
> That's not good.
> 
> If the program says '100ms' because it knows it will need to do a GUI update
> then, and you block it for 5 seconds because that's when the next value
> update happens, the user is stuck looking at their gkrellm or whatever not
> doing anything at all for 4.9 seconds....
> 
> This almost forces the use of multiple threads if the program wants to do
> its own timer management.

The application can use select() to wait both for any X events it needs
to service and for the data update at the same time, right?

-- 
Vojtech Pavlik
Director SuSE Labs

Re: Generic battery interface

[Shem Multinymous]

On 7/30/06, Valdis.Kletnieks@vt.edu <Valdis.Kletnieks@vt.edu> wrote:
> If the program says '100ms' because it knows it will need to do a GUI update
> then, and you block it for 5 seconds because that's when the next value
> update happens, the user is stuck looking at their gkrellm or whatever not
> doing anything at all for 4.9 seconds....
>
> This almost forces the use of multiple threads if the program wants to do
> its own timer management.

Please read my detailed proposal, posted (and resivsed) later.

The program is not blocked by the new ioctl, it still does a poll() or
select() and can provide a timeout, as usual. The only trick is that
the poll() won't return with an input-ready event until the
appropriate time.

  Shem

Re: Generic battery interface

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 1519 bytes --]

On Sun, 30 Jul 2006 14:48:07 +0300, Shem Multinymous said:
> On 7/30/06, Valdis.Kletnieks@vt.edu <Valdis.Kletnieks@vt.edu> wrote:
> > If the program says '100ms' because it knows it will need to do a GUI update
> > then, and you block it for 5 seconds because that's when the next value
> > update happens, the user is stuck looking at their gkrellm or whatever not
> > doing anything at all for 4.9 seconds....

> Please read my detailed proposal, posted (and resivsed) later.

OK, if you meant this one (that hadn't shown up here before I hit 'send'):

On Sun, 30 Jul 2006 13:14:10 +0300, Shem Multinymous said:
> Actually my solution was "any update but no sooner than N msecs". So
> you might be getting a readout that's N-1 msecs old, which was
> meanwhile cached by the driver. If you care about that, you need to
> use interleave those polls with msleep()s; see my recent detailed
> post. You'll still doing at most one msleep() per fetched readout,
> regardless of how frequently the driver provides them.

That has slightly different semantics indeed, and avoids the issue I
was commenting about.  A gkrellm-ish program can query every 100ms and
get a cached value 49 times out of 50 for a value that's hardware-updated
every 5 seconds, and all will be well (of course, there's room for some
added optimization, but I suspect trying to add that will end up more
expensive than just re-reading the same cached value, unless the kernel has
a good way to pass back a good hint of when the next update will be...)

[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: Generic battery interface

[Shem Multinymous]

On 7/31/06, Valdis.Kletnieks@vt.edu <Valdis.Kletnieks@vt.edu> wrote:
> OK, if you meant this one (that hadn't shown up here before I hit 'send'):

No, I ment this one: http://lkml.org/lkml/2006/7/30/193
(or actually its parent, at the time).


> was commenting about.  A gkrellm-ish program can query every 100ms and
> get a cached value 49 times out of 50 for a value that's hardware-updated
> every 5 seconds, and all will be well (of course, there's room for some
> added optimization,

That's exactly what we're trying to avoid -- excessive polling of
values that don't change. It causes unnecessary system load and timer
interrupts on tickless kernels.


> unless the kernel has
> a good way to pass back a good hint of when the next update will be...)

The kernel often doesn't know when it will get the next update. But on
the other hand, apps usually know well in advance  when they'll *want*
the next update. My proposal exploits this to get optimal behavior (if
the driver+infrastructure do the right thing).

  Shem

Re: Generic battery interface

[Pavel Machek]

Hi!

> > I do not understand this. Any polling (in kernel or in userspace) will
> > wake the CPU, wasting power.
> 
> The kernel, however, has all the gory details at hand, and can decide
> much better about the polling frequency, than the (hopefully) hardware
> agnostic userspace.
> 
> Imagine your Zaurus: You don't need to poll very often when you are on
> the flat part of the LiIon discharge curve, you probably want more
> detailed info near the end.

OTOH some applications just want more frequent polling than
others. Shem's "first update after N msec" solution looks most
flexible here.
								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/30/06, Pavel Machek <pavel@suse.cz> wrote:
> OTOH some applications just want more frequent polling than
> others. Shem's "first update after N msec" solution looks most
> flexible here.

Actually my solution was "any update but no sooner than N msecs". So
you might be getting a readout that's N-1 msecs old, which was
meanwhile cached by the driver. If you care about that, you need to
use interleave those polls with msleep()s; see my recent detailed
post. You'll still doing at most one msleep() per fetched readout,
regardless of how frequently the driver provides them.

Alternatively, we can add an extra parameter to that new
syscall/ioctl: "block until the time is T+N and you have a refresh
that was received from the hardware at time T+M, whichever is later"
(where T is the current time and N>M).

That's semantically equivalent to an msleep(M) followed by the
original delayed_update(N-M),  but will save one timer interrupt per
iteration in some cases (e.g., an event-based hardware data source).

  Shem

Re: Generic battery interface

[Shem Multinymous]

On 7/30/06, Shem Multinymous <multinymous@gmail.com> wrote:
> Alternatively, we can add an extra parameter to that new
> syscall/ioctl: "block until the time is T+N and you have a refresh
> that was received from the hardware at time T+M, whichever is later"
> (where T is the current time and N>M).
>
> That's semantically equivalent to an msleep(M) followed by the
> original delayed_update(N-M),  but will save one timer interrupt per
> iteration in some cases (e.g., an event-based hardware data source).

No, wait, the explicit two-parameter version is far superior.

First, it makes it easier to poll multiple attributes. How'd you do
that with the one-parameter version, if you need to sleep separately
for each attribute?

Second, it gives the driver (or its sysfs support infrastructure) a
better picture of the future, which it can use to optimize the
hardware query scheduling. No, I don't think anyone will implement
*that* soon, but it's good to have an API that lets us add it later
without affecting userspace.


Is anyone interested in adding this "delayed updates" support to
sysfs, for the benefit of hwmon, tp_smapi and probably numerous other
drivers?
I have some ideas for how to do it so that that per-driver support is
really easy (or even free), but I don't know sysfs well enough to
implement it and don't have the resources.

  Shem

Re: Generic battery interface

[Dmitry Torokhov]

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

4) sysfs - all capabilities, IDs, etc for input devices exported there as well.

-- 
Dmitry

Re: Generic battery interface

[Vojtech Pavlik]

On Fri, Jul 28, 2006 at 08:25:35AM -0400, Dmitry Torokhov wrote:
> On 7/28/06, Vojtech Pavlik <vojtech@suse.cz> wrote:
> >On Fri, Jul 28, 2006 at 03:27:00AM +0300, Shem Multinymous wrote:
> >>
> >> "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
> >
> 
> 4) sysfs - all capabilities, IDs, etc for input devices exported there as 
> well.
 
Oh, of course. I don't know how I could forget sysfs here - I had it in
mind when I started writing the list ...

-- 
Vojtech Pavlik
Director SuSE Labs

Re: Generic battery interface

[Shem Multinymous]

On 7/28/06, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> 4) sysfs - all capabilities, IDs, etc for input devices exported there as well.

Forgive my ignorance, but how do I conncet a sysfs directory with a /dev device?
So far the only way I found is to compare /sys/whatever/dev with the
major+minor of the dev file, which requires brute-force enumeration on
at least one side.

  Shem

Re: Generic battery interface

[Dmitry Torokhov]

On 7/28/06, Shem Multinymous <multinymous@gmail.com> wrote:
> On 7/28/06, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > 4) sysfs - all capabilities, IDs, etc for input devices exported there as well.
>
> Forgive my ignorance, but how do I conncet a sysfs directory with a /dev device?
> So far the only way I found is to compare /sys/whatever/dev with the
> major+minor of the dev file, which requires brute-force enumeration on
> at least one side.
>

Can't we have udev make a symlink when it creates device node? Kernel
can't provide this link since it knows nothing about naming. You could
either have a symlink from /sys/ or maybe /dev/enum/<name matching
print_dev_t() output>

-- 
Dmitry

Re: Generic battery interface

[Greg KH]

On Fri, Jul 28, 2006 at 12:10:43PM -0400, Dmitry Torokhov wrote:
> On 7/28/06, Shem Multinymous <multinymous@gmail.com> wrote:
> >On 7/28/06, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> >> 4) sysfs - all capabilities, IDs, etc for input devices exported there 
> >as well.
> >
> >Forgive my ignorance, but how do I conncet a sysfs directory with a /dev 
> >device?
> >So far the only way I found is to compare /sys/whatever/dev with the
> >major+minor of the dev file, which requires brute-force enumeration on
> >at least one side.
> >
> 
> Can't we have udev make a symlink when it creates device node?

No, ick, why would you want that?

Just look at the "dev" file in sysfs, which shows the major:minor
number.

Or just look at the directory that you are in, and that's almost always
the /dev node name.

For example, /sys/block/sda/sda1/ is /dev/sda1.
/sys/class/tty/ttyS1 is /dev/ttyS1.

It's usually not that difficult to do the mapping :)

thanks,

greg k-h

Re: Generic battery interface

[Shem Multinymous]

On 7/30/06, Greg KH <greg@kroah.com> wrote:
> > >Forgive my ignorance, but how do I conncet a sysfs directory with a /dev
> > >device?

> Just look at the "dev" file in sysfs, which shows the major:minor
> number.

Then to find the match for a given device node you need to enumerate /sys.
And to find a match for a given /sys device you need to enumerate
/dev, or some its subdirectories (/dev/{snd,input,...), or whatever
other random places people have decided to place their device nodes.


> Or just look at the directory that you are in, and that's almost always
> the /dev node name.
>
> For example, /sys/block/sda/sda1/ is /dev/sda1.
> /sys/class/tty/ttyS1 is /dev/ttyS1.

Yeah, and /sys/block/sr0 is /dev/scd0 (FC5 default udev rules).


> It's usually not that difficult to do the mapping :)

Hmm, "usually"...


Coming to think of it, to solve the dev->sys direction, maybe we
should have symlinks like the following?
/sys/dev/8/0 -> /sys/block/sda
/sys/dev/11/0 -> /sys/block/sr0
/sys/dev/116/24 -> /sys/class/sound/pcmC0D0c


Put otherwise:
Q:Quick, which io scheduler is used by /dev/scd0?
A: cat /sys/dev/$((0x`stat -c%t /dev/scd0`))/\
                $((0x`stat -c%T /dev/scd0`))/queue/scheduler

(Please excuse the ugly hex to dec conversion.)


  Shem

Re: Generic battery interface

[Vojtech Pavlik]

On Sun, Jul 30, 2006 at 12:52:52PM +0300, Shem Multinymous wrote:

> Coming to think of it, to solve the dev->sys direction, maybe we
> should have symlinks like the following?
> /sys/dev/8/0 -> /sys/block/sda
> /sys/dev/11/0 -> /sys/block/sr0
> /sys/dev/116/24 -> /sys/class/sound/pcmC0D0c

Since you can have more nodes in /dev with the same node numbers, and
this actually is useful (for granting more users/groups access to the
devices in question), this is not going to fly.

-- 
Vojtech Pavlik
Director SuSE Labs

Re: Generic battery interface

[Shem Multinymous]

On 7/30/06, Vojtech Pavlik <vojtech@suse.cz> wrote:
> On Sun, Jul 30, 2006 at 12:52:52PM +0300, Shem Multinymous wrote:
>
> > Coming to think of it, to solve the dev->sys direction, maybe we
> > should have symlinks like the following?
> > /sys/dev/8/0 -> /sys/block/sda
> > /sys/dev/11/0 -> /sys/block/sr0
> > /sys/dev/116/24 -> /sys/class/sound/pcmC0D0c
>
> Since you can have more nodes in /dev with the same node numbers, and
> this actually is useful (for granting more users/groups access to the
> devices in question), this is not going to fly.

No, I'm talking about the *other* direction now: I'm looking at a file
in /dev and I want to find the corresponding sysfs device (if any).

Anyway turns out that sysfs dev numbers are not unique either -- some
devices with major=1 apper twice in sysfs. To give a random example:

$ cat /sys/block/ram8/dev /sys/class/mem/random/dev
1:8
1:8

And these aren't symlinks.
  Shem

Re: Generic battery interface

[Tomasz Torcz]

[-- Attachment #1: Type: text/plain, Size: 450 bytes --]

On Sun, Jul 30, 2006 at 12:52:52PM +0300, Shem Multinymous wrote:
> Put otherwise:
> Q:Quick, which io scheduler is used by /dev/scd0?
> A: cat /sys/dev/$((0x`stat -c%t /dev/scd0`))/\
>                $((0x`stat -c%T /dev/scd0`))/queue/scheduler


 A2: cat /sys`udevinfo -n scd0 -q path`/queue/scheduler  ?

-- 
Tomasz Torcz                 "God, root, what's the difference?"
zdzichu@irc.-nie.spam-.pl         "God is more forgiving."


[-- Attachment #2: Type: application/pgp-signature, Size: 229 bytes --]

Re: Generic battery interface

[Shem Multinymous]

On 7/30/06, Tomasz Torcz <zdzichu@irc.pl> wrote:
> On Sun, Jul 30, 2006 at 12:52:52PM +0300, Shem Multinymous wrote:
> > Put otherwise:
> > Q:Quick, which io scheduler is used by /dev/scd0?
> > A: cat /sys/dev/$((0x`stat -c%t /dev/scd0`))/\
> >                $((0x`stat -c%T /dev/scd0`))/queue/scheduler
>
>  A2: cat /sys`udevinfo -n scd0 -q path`/queue/scheduler  ?

You win.
Since udev manages /dev, it makes sense to ask it for this info. I
guess one can think of major:minor as a udev private implementation
detail, in this context.

  Shem

Re: Generic battery interface

[Vojtech Pavlik]

On Sun, Jul 30, 2006 at 01:55:00AM -0700, Greg KH wrote:

> No, ick, why would you want that?
> 
> Just look at the "dev" file in sysfs, which shows the major:minor
> number.
> 
> Or just look at the directory that you are in, and that's almost always
> the /dev node name.
> 
> For example, /sys/block/sda/sda1/ is /dev/sda1.
> /sys/class/tty/ttyS1 is /dev/ttyS1.
> 
> It's usually not that difficult to do the mapping :)
 
But it's wrong to rely on it API-wise. 

-- 
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

[Pavel Machek]

Hi!

> >> 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.

I still like /sys approach a bit more, but a word of warning: "it is
hard to change kernel<->user interface" is actually a feature.

It sucks when you are the one doing the work, but maybe it will mean
reusing interfaces where possible.

								Pavel

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

Re: Generic battery interface

[Vojtech Pavlik]

On Sat, Jul 29, 2006 at 01:48:34AM +0300, Shem Multinymous wrote:
> 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.

Sure. What I want is an interface that encourages good apps.

> >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.

Right. And I don't care much which of them will be the final
implementation.

The main difference that remains is that with /dev, all the immediate
data and the events come from the same source - the device file.

For /sys, you need to open multiple files, and re-read them either based
on information from yet another file or select()ing on them all. But
this may not be a huge problem.

> >> 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.

It's nicer not to have to worry about that, I agree.

> >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).

I think we're hitting a fundamental problem with sysfs/hotplug/udev
here. It was created to get fixed, non-changing names of devices in
/dev, so that they'd be easy to enter into configuration files.

Yet applications today want automatic discovery of devices and don't
want to rely on udev getting the names right. 

We should make our minds up, and decide whether we want the 'devices are
in /dev and applications just need to open the filename' or the 'an
application will find the device itself' approach.

This reminds me very much of the Joerg Schilling discussion (flamewar)
of enumerating CD-burners. Most people on the kernel mailing list just
wanted to enter the name of the device node on the cdrecord command
line. Yet Joerg insisted that the application should do the discovery.

Here we're doing exactly the same: Where simply specifying a rule in
udev, and a fixed name to the battery applet would have worked fine,
people are trying to design ways how to do the discovery from the
applet.

I still believe the first approach is the only one that is sane and
portable.

> >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. :-)

HDAPS, as explained above, doesn't have huge latency impact. The reason
to have high update rates for input devices (mice nowadays run at 100 Hz
refresh usually, gaming mice up to 1 kHz), is to not introduce
additional delay to the user->computer->user closed control loop.

The less delay, the better stability of the control loop and the better
results in the game. The limiting factor is usually 3D rendering, but a
10 Hz joystick will still kill the experience by inducing a much larger
delay.

> 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*".

Sort of a 'reverse select'. Interesting idea.

> 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.
 

-- 
Vojtech Pavlik
Director SuSE Labs

Re: Generic battery interface

[Shem Multinymous]

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

> I think we're hitting a fundamental problem with sysfs/hotplug/udev
> here. It was created to get fixed, non-changing names of devices in
> /dev, so that they'd be easy to enter into configuration files.
>
> Yet applications today want automatic discovery of devices and don't
> want to rely on udev getting the names right.
>
> We should make our minds up, and decide whether we want the 'devices are
> in /dev and applications just need to open the filename' or the 'an
> application will find the device itself' approach.

I think what people want from device choice is a reasonable default
plus a convenient way to override things. The former is handled nicely
by distributions' udev rules, while the latter is best done by
providing fixed paths. As an end-user, if I know my favorite joystick
is on a specific USB port (hence a specific syfs directory), then I
want to tell neverball "use that one" without setting up nasty udev
rules or playing major:minor matchup. Yes, that's bypassing the Proper
Udevian Way of Doing Things, but it's so much easier and Unix-like
that we really should make it possible (though not by default!).

Security issues aside (for a moment):
Is there any reason not to provide real device inodes on sysfs,
instead of just a textual /sys/foo/dev? And then, maybe udev should
symlink to those device files under /sys instead of creating its own?
This would tie the two systems together rather elegantly.


> This reminds me very much of the Joerg Schilling discussion (flamewar)
> of enumerating CD-burners. Most people on the kernel mailing list just
> wanted to enter the name of the device node on the cdrecord command
> line. Yet Joerg insisted that the application should do the discovery.

I think there's a lot more to *that* flamewar - such as unwavering
belief in generic scsi...


> > If you rattle your ThinkPad this badly, latency in Neverball is going
> > to be the least of your problems. :-)
>
> HDAPS, as explained above, doesn't have huge latency impact. The reason
> to have high update rates for input devices (mice nowadays run at 100 Hz
> refresh usually, gaming mice up to 1 kHz), is to not introduce
> additional delay to the user->computer->user closed control loop.
>
> The less delay, the better stability of the control loop and the better
> results in the game. The limiting factor is usually 3D rendering, but a
> 10 Hz joystick will still kill the experience by inducing a much larger
> delay.

Yes, I understand. I just pointed out that in the specific case of
system accelerometer readouts, either the readouts change very slowly
or your laptop is being rattled into an early death.


> Sort of a 'reverse select'.

Exactly.


  Shem

Re: Generic battery interface

[Vojtech Pavlik]

On Sat, Jul 29, 2006 at 02:32:02PM +0300, Shem Multinymous wrote:
> On 7/29/06, Vojtech Pavlik <vojtech@suse.cz> wrote:
> 
> >I think we're hitting a fundamental problem with sysfs/hotplug/udev
> >here. It was created to get fixed, non-changing names of devices in
> >/dev, so that they'd be easy to enter into configuration files.
> >
> >Yet applications today want automatic discovery of devices and don't
> >want to rely on udev getting the names right.
> >
> >We should make our minds up, and decide whether we want the 'devices are
> >in /dev and applications just need to open the filename' or the 'an
> >application will find the device itself' approach.
> 
> I think what people want from device choice is a reasonable default
> plus a convenient way to override things. The former is handled nicely
> by distributions' udev rules, while the latter is best done by
> providing fixed paths. As an end-user, if I know my favorite joystick
> is on a specific USB port (hence a specific syfs directory), then I
> want to tell neverball "use that one" without setting up nasty udev
> rules or playing major:minor matchup. Yes, that's bypassing the Proper
> Udevian Way of Doing Things, but it's so much easier and Unix-like
> that we really should make it possible (though not by default!).

IMO the right way here would be to have a nice GUI for configuring udev
included with the distro, that'd let you browse the sysfs tree and
point'n'click to create the rule you need.

> Security issues aside (for a moment):
> Is there any reason not to provide real device inodes on sysfs,
> instead of just a textual /sys/foo/dev? And then, maybe udev should
> symlink to those device files under /sys instead of creating its own?
> This would tie the two systems together rather elegantly.

The reason behind this was to force people NOT use sysfs directly when
interfacing to the OS. ;)

Because sysfs wasn't intended to be an API you can rely on, one that's
fixed in stone and cannot be changed for compatibility reasons. I
believe it failed in that respect.

> >This reminds me very much of the Joerg Schilling discussion (flamewar)
> >of enumerating CD-burners. Most people on the kernel mailing list just
> >wanted to enter the name of the device node on the cdrecord command
> >line. Yet Joerg insisted that the application should do the discovery.
> 
> I think there's a lot more to *that* flamewar - such as unwavering
> belief in generic scsi...

Sure. It was just one of the points raised there.

> >HDAPS, as explained above, doesn't have huge latency impact. The reason
> >to have high update rates for input devices (mice nowadays run at 100 Hz
> >refresh usually, gaming mice up to 1 kHz), is to not introduce
> >additional delay to the user->computer->user closed control loop.
> >
> >The less delay, the better stability of the control loop and the better
> >results in the game. The limiting factor is usually 3D rendering, but a
> >10 Hz joystick will still kill the experience by inducing a much larger
> >delay.
> 
> Yes, I understand. I just pointed out that in the specific case of
> system accelerometer readouts, either the readouts change very slowly
> or your laptop is being rattled into an early death.

You want the frequent readouts even for slow changes of the direction of
gravity there, that's what I wanted to say.

> >Sort of a 'reverse select'.
> 
> Exactly.

-- 
Vojtech Pavlik
Director SuSE Labs

Re: Generic battery interface

[Shem Multinymous]

On 7/29/06, Vojtech Pavlik <vojtech@suse.cz> wrote:
> > I think what people want from device choice is a reasonable default
> > plus a convenient way to override things. The former is handled nicely
> > by distributions' udev rules, while the latter is best done by
> > providing fixed paths. As an end-user, if I know my favorite joystick
> > is on a specific USB port (hence a specific syfs directory), then I
> > want to tell neverball "use that one" without setting up nasty udev
> > rules or playing major:minor matchup. Yes, that's bypassing the Proper
> > Udevian Way of Doing Things, but it's so much easier and Unix-like
> > that we really should make it possible (though not by default!).
>
> IMO the right way here would be to have a nice GUI for configuring udev
> included with the distro, that'd let you browse the sysfs tree and
> point'n'click to create the rule you need.

That's still an extra level of indirection. You have to use the nice
GUI to create a new /dev/something, and then point your at at dev
/dev/something. And you have to be root to do that, whereas some sysfs
stuff is world-readable.


> > Security issues aside (for a moment):
> > Is there any reason not to provide real device inodes on sysfs,
> > instead of just a textual /sys/foo/dev? And then, maybe udev should
> > symlink to those device files under /sys instead of creating its own?
> > This would tie the two systems together rather elegantly.
>
> The reason behind this was to force people NOT use sysfs directly when
> interfacing to the OS. ;)
>
> Because sysfs wasn't intended to be an API you can rely on, one that's
> fixed in stone and cannot be changed for compatibility reasons. I
> believe it failed in that respect.

Is sysfs supposed to be a private" API that only "special services
services" look at? It has definitely failed in this respect -- It's
just too convenient and attractive. I'm not sure that's a bad thing...

Given the current usage pattern of sysfs, is it still a bad idea for
it to carry device inodes?

  Shem

Re: Generic battery interface

[Vojtech Pavlik]

On Sat, Jul 29, 2006 at 03:51:45PM +0300, Shem Multinymous wrote:

> >IMO the right way here would be to have a nice GUI for configuring udev
> >included with the distro, that'd let you browse the sysfs tree and
> >point'n'click to create the rule you need.
> 
> That's still an extra level of indirection. You have to use the nice
> GUI to create a new /dev/something, and then point your at at dev
> /dev/something. And you have to be root to do that, whereas some sysfs
> stuff is world-readable.

If that app opens /dev/something by default, which is usually the case,
there is only one step.

> >The reason behind this was to force people NOT use sysfs directly when
> >interfacing to the OS. ;)
> >
> >Because sysfs wasn't intended to be an API you can rely on, one that's
> >fixed in stone and cannot be changed for compatibility reasons. I
> >believe it failed in that respect.
> 
> Is sysfs supposed to be a private" API that only "special services
> services" look at? It has definitely failed in this respect -- It's
> just too convenient and attractive. I'm not sure that's a bad thing...

I believe it was originally intended as a cleaner replacement for procfs
- to allow the kernel export information about itself in a clean, safe,
and consistent way. It wasn't intended for data delivery. 

I don't know whether the current state of things is good or bad.

> Given the current usage pattern of sysfs, is it still a bad idea for
> it to carry device inodes?
 
That remains an open question.

-- 
Vojtech Pavlik
Director SuSE Labs

Re: Generic battery interface

[Greg KH]

On Sat, Jul 29, 2006 at 03:42:25PM +0200, Vojtech Pavlik wrote:
> On Sat, Jul 29, 2006 at 03:51:45PM +0300, Shem Multinymous wrote:
> > Given the current usage pattern of sysfs, is it still a bad idea for
> > it to carry device inodes?
>  
> That remains an open question.

No, that's a closed question.  It simply will not happen.  Linus vetoed
this years ago, for lots of different reasons that I can't recall at the
moment (namespaces, permissions, groups, and other things from what I
remember.)

Search the lkml archives if you are really curious...

thanks,

greg k-h

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

[Pavel Machek]

Hi!

> >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.

Looks good to me.

> 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.

I guess that hwmon people would like this, anyway...

Are there any plans at merging tp_smapi, BTW? After fixing few minor
details (like removing " mV" from files)... it looks like it would fit
into hwmon infrastructure rather nicely.
								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/30/06, Pavel Machek <pavel@suse.cz> wrote:

> Are there any plans at merging tp_smapi, BTW? After fixing few minor
> details (like removing " mV" from files)... it looks like it would fit
> into hwmon infrastructure rather nicely.

Yes, it will definitely be submitted once stable. We're still tweaking
some stuff, such as whitelisting (see "[PATCH] DMI: Decode and save
OEM String information").

BTW, I'll rename the modules to "thinkpad_ec" for basic EC functions
(also used by the patched hdaps), and probably "thinkpad_battery" or
"thinkpad_hwmon" for the rest. Not sure about the latter: we happen to
be doing only battery stuff at the moment but this is likely to change
in the future.

Is "hwmon" suitable for something that *contols* battery charging too,
and might have extra attributes tacked on as we unravel additional
ThinkPad firmware capabilities?

  Shem

Re: Generic battery interface

[Pavel Machek]

Hi!

> >Are there any plans at merging tp_smapi, BTW? After fixing few minor
> >details (like removing " mV" from files)... it looks like it would fit
> >into hwmon infrastructure rather nicely.
> 
> Yes, it will definitely be submitted once stable. We're still tweaking
> some stuff, such as whitelisting (see "[PATCH] DMI: Decode and save
> OEM String information").

Please avoid this trap. You probably want to submit it early. "Submit
once stable" is dangerous: cleanups needed while submitting sometimes
make the whole work unstable, again.

> Is "hwmon" suitable for something that *contols* battery charging too,
> and might have extra attributes tacked on as we unravel additional
> ThinkPad firmware capabilities?

I'd say it is okay. AFAICT, hwmon can already control fan.
									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]

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

Re: Generic battery interface

[Jean Delvare]

Hi Shem,

Disclaimer: I have no idea how the input interface works currently. And
I don't know what problem you are trying to solve. I just thought my
hwmon-oriented comments might help.

> 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.

"A or B, whichever is later", effectively means "A and B". Or am I
missing something? I fail to see the difference between min_wait and
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.

The hwmon interface (sysfs now, procfs before) has been returning
cached values by defaut for years. Changing this at this point might be
confusing. I don't see much benefit in waiting for updated values
compared to reading them from the cache. The driver knows better how
frequently it can read from the chip. And no hardware monitoring chip I
know of can tell when the monitored value has changed - you have to read
the chip registers to know.

> 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.

I don't see the point in the 700ms rule. If you don't want new data
more often than once per second, the readouts will be spaced by one
second, which implies > 700ms already.

> 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: */

No seek(fd, ..., 0) here? sysfs files are supposed to be simple text
files, aren't they?

> 	read(fd, ...);
> 
> 	... handle readout, update GUI ...
> }

-- 
Jean Delvare

Re: Generic battery interface

[Shem Multinymous]

Hi Jean,

On 8/1/06, Jean Delvare <khali@linux-fr.org> wrote:
> Disclaimer: I have no idea how the input interface works currently. And
> I don't know what problem you are trying to solve. I just thought my
> hwmon-oriented comments might help.

Your comments are highly relevant! Almost everything said in this
thread, apart from "where should we put battery readouts", is
applicable to hwmon.

The problem we're trying to solve is minimizing system load and timer
interrupts caused by apps that track kernel-issued readouts (e.g.,
hwmon's), without the application making unnecessary assumptions about
the driver or vice versa.


> > 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.
>
> "A or B, whichever is later", effectively means "A and B". Or am I
> missing something?

The equivlent phrasing using "and" is s follows:

"as soon as the current time is at least T+min_wait *and* you have a
readout that was received from the hardware at  time at least
T+min_fresh."


> I fail to see the difference between min_wait and min_fresh.

Here's an example illustrating all parameters.

Suppose the app does a DELAYED_UPDATE with min_wait=700 and
min_fresh=1000 at time T, and then poll()s with a timeout of 9999.

At time T+600 the driver receives update events (yeah, it' that kind
of driver). If this is all that happens, the app will remain blocked
for 9999ms -- the requirement for a readout fresher than T+700 is
never fulfilled.

If the driver get a second event at time T+800, the app will be
unblocked at time T+1000. If, instead, the driver gets the second
event at time T+1200, the app will be unblocked immediately.


> The hwmon interface (sysfs now, procfs before) has been returning
> cached values by defaut for years. Changing this at this point might be
> confusing.

Yes. All those should be told to use O_NONBLOCK, or be delayed by one
readout cycle whenever they poll an attribute. How serious is this?

The essential problem is that we don't want drivers to query the
hardware and cause timer interrups when nobody's listening, so cached
values can get arbitrarily stale. Thus, an out-of-the-blue read by a
*must* wait for a refresh (i.e., hardware query). I don't see a
non-kludgy way out of it.


> I don't see much benefit in waiting for updated values
> compared to reading them from the cache. The driver knows better how
> frequently it can read from the chip.

Yes, but the driver doesn't know how frequently apps *need* it to read
from the chip.

Take the hdaps driver, for example. The hardware can provide fresh
readouts at a rate of 500Hz. Some apps (e.g., hdapsd) actually need a
high poll rate. Others (e.g., joystick emulation and hdaps-gl) work
nicely with 20Hz. And the tp-theft app will be useful even with 1Hz,
and may change its poll rate depending on circumstances. So, what is
the driver supposed to do?

In my proposal, the the driver and all app (implicitly) negotiate a
poll rate which makes sense for all parties involved.


> And no hardware monitoring chip I
> know of can tell when the monitored value has changed - you have to read
> the chip registers to know.

Yes, this may not be relevant for traditional hwmon chips, but we're
trying to handle event-based data sources too. You might get an ACPI
event on temperature change, or a "critical temperature" interrupt
(which flips a boolean sysfs attr) , etc. I can't think of a really
convincing example, but we certainly want an interface that will
support such drivers transparently to userspace.


> > 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.
>
> I don't see the point in the 700ms rule. If you don't want new data
> more often than once per second, the readouts will be spaced by one
> second, which implies > 700ms already.

Only on average. If you use min_wait=1000 and min_fresh=0, you might
be seeing cached readouts that were obtained at times
999,1001,2999,3001,4999,5001,...
which is probably not what you want.


> No seek(fd, ..., 0) here? sysfs files are supposed to be simple text
> files, aren't they?

Yes, for sysfs there's a seek(fd, ..., 0). There isn't one if you use
the input infrastructure's approach, where read()s return a stream of
event records.

  Shem

Re: Generic battery interface

[Pavel Machek]

Hi!

> frequently it can read from the chip. And no hardware monitoring chip I
> know of can tell when the monitored value has changed - you have to read
> the chip registers to know.

ACPI battery can tell when values change in significant way. (Like
battery becoming critical).

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

Re: Generic battery interface

[Jean Delvare]

Hi Pavel,

> > frequently it can read from the chip. And no hardware monitoring chip I
> > know of can tell when the monitored value has changed - you have to read
> > the chip registers to know.
> 
> ACPI battery can tell when values change in significant way. (Like
> battery becoming critical).

Ah, good to know. But is there a practical use for this? I'd suspect
that the user wants to know the battery charge% all the time anyway,
critical or not.

-- 
Jean Delvare

Re: Generic battery interface

[Marek Wawrzyczny]

On Wednesday 02 August 2006 17:18, Jean Delvare wrote:
> Hi Pavel,
>
> > > frequently it can read from the chip. And no hardware monitoring chip I
> > > know of can tell when the monitored value has changed - you have to
> > > read the chip registers to know.
> >
> > ACPI battery can tell when values change in significant way. (Like
> > battery becoming critical).
>
> Ah, good to know. But is there a practical use for this? I'd suspect
> that the user wants to know the battery charge% all the time anyway,
> critical or not.

Yes, the user may want to know the battery state all the time, but will not 
notice the difference between the system reporting battery changes every 100 
microseconds or every 10 seconds, unless the hardware eats its battery 
sources for breakfast? 

The system cares though, very much so in fact:

If the battery becomes critical the system should either shut down or suspend 
to disk (if this is supported). 
This obviously would be triggered by some sort of daemon (powersaved comes to 
mind).
Ideally the suspend to disk or shutdown event triggers another event that 
allows any desktop to save it's state or possibly unmount shares that could 
otherwise be corrupted.

This scenario is quite different to an application reading the battery level.


Marek Wawrzyczny

Re: Generic battery interface

[Thomas Renninger]

On Wed, 2006-08-02 at 09:18 +0200, Jean Delvare wrote:
> Hi Pavel,
> 
> > > frequently it can read from the chip. And no hardware monitoring chip I
> > > know of can tell when the monitored value has changed - you have to read
> > > the chip registers to know.
> > 
> > ACPI battery can tell when values change in significant way. (Like
> > battery becoming critical).
> 
> Ah, good to know. But is there a practical use for this? I'd suspect
> that the user wants to know the battery charge% all the time anyway,
> critical or not.

Some batteries throw an event for each consumed percent or at least
enough events to keep track of remaining power.
Some are only throwing an event when capacity warning/low is reached,
some aren't throwing any events.

It may be of use to reduce polling on some machines, but you will always
need a fall back. Determining whether the machine throws events
regularly is not really possible, so by default you will start to poll
the battery on all machines. Maybe in this case the normal (0x80)
battery events should be ignored to avoid double readings or the values
are cached in kernel as suggested, then it does not hurt.

One should also not rely on the warning/low capacity values.
I cannot say for sure whether all machines throw an event if these
limits are reached. We defined our own limits in userspace, this always
works. I'd go for not using the BIOS limits here at all and take user
defined capacity warning/low values (in percent) in hal or wherever.

IMO opinion the normal battery events (0x80) are not much of a use. They
probably should be forwarded, so that userspace could switch to irq
driven notification if this should ever work on more than 90% of the
machines, but default will be polling. More important are the status
events (0x81) if a battery is added/removed.

   Thomas

Re: Generic battery interface

[Pavel Machek]

Hi!

> One should also not rely on the warning/low capacity values.
> I cannot say for sure whether all machines throw an event if these
> limits are reached. We defined our own limits in userspace, this always
> works. I'd go for not using the BIOS limits here at all and take user
> defined capacity warning/low values (in percent) in hal or wherever.

Well, this works okay for normal machines, but... there are strange
machines around.

If you have 10 hours of battery life (zaurus sl-5500) and hardcode
warning to 10% of battery remainin... well you'll warn with 1 hour to
go.

Okay, this probably is not an issue on most notebooks today...

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

Re: Generic battery interface

[Pavel Machek]

Hi!

> >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.

Agreed.

> 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.

Well, we have prior examples for /dev/XXX style interface (input), and
we have examples for /sys/XXX interface (LED subsystem). Each has its
advantages and disadvantages, and both are okay for programming
AFAICT.

> 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:-)

Let me clarify this: zaurus-style systems can read battery status as
often as someone asks, and do not really have events. If someone asks,
they read the status. If noone asks, they do not have to read status
_at all_. Event interface suits PC world where BIOS already does the
polling...

Anyway, let's compare /dev/XXX vs. /sys/XXX

/sys/XXX:
+ existing code uses similar scheme
+ easy to add very obscure features, such as 
	do_not_charge_battery_for_X_minutes
+ perhaps it would not need explicit maintainer, just assign names 
	carefully
- lack of maintainer mean people will probably not assign names
	carefully
- does not suit PC-style batteries which trigger events when data
	change (can be fixed by /sys/XXX/anything-new, which gives one
	byte when something changes)
- you are not getting atomic snapshot of battery state. For example
	you could read battery status okay, voltage 9.5V; while real
	states were (okay, 10.5V), (critical, 9.5V) and update
	happened between you reading status and voltage. (Is it
	problem?)
+ userspace UPS daemons can just create files somewhere else, battery
	applets can scan two directories easily

/dev/XXX
+ battery layer will need to be invented
- that layer will need maintainer
+ maintainer ensures this is not a mess, allocates numbers
- does not quite suit zaurus-style batteries, that can _only_ be
	polled. Can be worked-around by polling from kernel (and we
	are often doing that, anyway)
+ userspace backdoor interface will need to be invented, /dev/uinput style
- hard to handle obscure features
	(do_not_charge_battery_for_X_minutes), and we'll probably want
	to unify batteries a bit, probably loosing precision.

So... I'm not sure which one I prefer. If I had to do one _myself_,
I'd do /sys/XXX version, because it requires less maintainance, and
I'm lazy.

If I could clone vojtech, I'd prefer just doing that, then letting him
do /dev/XXX interface. It will be more work and painful transition.

Anyone volunteers write battery layer? If so, I'd go with /dev/XXX,
otherwise I'd go with /sys/XXX, because it is simpler to code, and I
believe it is also good enough...
									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/28/06, Pavel Machek <pavel@suse.cz> wrote:

> Event interface suits PC world where BIOS already does the
> polling...

The BIOS does polling only for its internal tasks. It doesn't generate
events when readouts change unless something important happened. So if
you want readouts you need to poll the SMBIOS or hardware, like on the
Zaurus.


> + perhaps it would not need explicit maintainer, just assign names
>         carefully

We also need to decide on clear convention about units. Are they in
the output and/or filename? Filename is best, I think, since it's
impossible to miss and works nicely for input attributes too.


> - does not suit PC-style batteries which trigger events when data
>         change (can be fixed by /sys/XXX/anything-new, which gives one
>         byte when something changes)

Changed since last poll? That doesn't work with multiple clients.
Changed for the last X seconds? That requires everybody to poll that
frequenty, and risks missing events due to system load.

Wild thought: how about adding a generic "event source" mechanism into
sysfs, at the same level as attributes? Maybe even make them textual,
in keeping with sysfs philosophy:

while read TYPE PARAM  < /sys/class/battery/BAT0/criticl_events; do
  echo "battery 0 generated ctitical event $TYPE with parameters $PARAM"
done

The simpler solution is to convert events into state (e.g.,
critical=0/1) and present them as normal attributes which userspace
can poll, as Greg KH suggested (did I get that right?). I'm not sure
it's always easy, e.g., does ACPI genereate an explicit
no-longer-critical event?


> - you are not getting atomic snapshot of battery state. For example
>         you could read battery status okay, voltage 9.5V; while real
>         states were (okay, 10.5V), (critical, 9.5V) and update
>         happened between you reading status and voltage. (Is it
>         problem?)

I can't think of a realistic scenario where it's a problem. The latest
readout is always the best one to look at.


> - hard to handle obscure features
>         (do_not_charge_battery_for_X_minutes)

So where does that go in the /dev scheme?


  Shem

Re: Generic battery interface

[Greg KH]

On Fri, Jul 28, 2006 at 01:56:03AM +0300, Shem Multinymous wrote:
> On 7/28/06, Pavel Machek <pavel@suse.cz> wrote:
> >+ perhaps it would not need explicit maintainer, just assign names
> >        carefully
> 
> We also need to decide on clear convention about units. Are they in
> the output and/or filename? Filename is best, I think, since it's
> impossible to miss and works nicely for input attributes too.

Actually, this whole thing could probably just go under the 'hwmon'
interface, as it already handles other hardware monitoring events.  I
don't see how a battery would be any different, do you?

> >- does not suit PC-style batteries which trigger events when data
> >        change (can be fixed by /sys/XXX/anything-new, which gives one
> >        byte when something changes)
> 
> Changed since last poll? That doesn't work with multiple clients.
> Changed for the last X seconds? That requires everybody to poll that
> frequenty, and risks missing events due to system load.

Again, look at the hwmon documentation, they handle alarms and other
things already that you are trying to re-invent.

> Wild thought: how about adding a generic "event source" mechanism into
> sysfs, at the same level as attributes? Maybe even make them textual,
> in keeping with sysfs philosophy:
> 
> while read TYPE PARAM  < /sys/class/battery/BAT0/criticl_events; do
>  echo "battery 0 generated ctitical event $TYPE with parameters $PARAM"
> done

Heh, no, the file should specify the units, and then you document it.
Much simpler.

> The simpler solution is to convert events into state (e.g.,
> critical=0/1) and present them as normal attributes which userspace
> can poll, as Greg KH suggested (did I get that right?).

Yes, just like temperature events today.

People have asked for the "this sysfs file's value changed" type uevent
message to come back, so that's also an option that might be used here.

thanks,

greg k-h

Re: Generic battery interface

[Pavel Machek]

Hi!

> > >+ perhaps it would not need explicit maintainer, just assign names
> > >        carefully
> > 
> > We also need to decide on clear convention about units. Are they in
> > the output and/or filename? Filename is best, I think, since it's
> > impossible to miss and works nicely for input attributes too.
> 
> Actually, this whole thing could probably just go under the 'hwmon'
> interface, as it already handles other hardware monitoring events.  I
> don't see how a battery would be any different, do you?

Heh... yes, hwmon already has voltage, current, and more importantly,
a maintainer.

I'd still prefer batteries to go into /sys/class/battery/... they are
really different from lm78-style voltage sensor and I'd not expect
battery applet to understand all the fields "normal" hwmon
exports. But conventions developed by hwmon group look sane and
usable.

Actually I do not see "hwmon infrastructure" to exist. Every driver
just uses sysfs directly. I'm not sure that the best option --
"input-like" infrastructure can make drivers even shorter -- but
perhaps just directly using sysfs is best for simple task like a battery?

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

Re: Generic battery interface

[Jean Delvare]

Hi Pavel,

> > > We also need to decide on clear convention about units. Are they in
> > > the output and/or filename? Filename is best, I think, since it's
> > > impossible to miss and works nicely for input attributes too.
> > 
> > Actually, this whole thing could probably just go under the 'hwmon'
> > interface, as it already handles other hardware monitoring events.  I
> > don't see how a battery would be any different, do you?
> 
> Heh... yes, hwmon already has voltage, current, and more importantly,
> a maintainer.
> 
> I'd still prefer batteries to go into /sys/class/battery/... they are
> really different from lm78-style voltage sensor and I'd not expect
> battery applet to understand all the fields "normal" hwmon
> exports.

This probably doesn't matter much, every application can handle only
the subset it is interested in. For example a dockapp displaying the
system temperature only cares about the temp* files and ignores the
in*, fan*, etc. files.

However, it would be convenient if the battery monitoring application
had an easy (and non heuristic-based) way to distinguish between a
battery and a hardware monitoring chip. In that sense, having a
separate class makes sense. This doesn't prevent the battery drivers to
use the same conventions used by the hardware monitoring drivers.

> But conventions developed by hwmon group look sane and usable.

Nice to read this for a change, usually people have only complaints
about it ;)

> Actually I do not see "hwmon infrastructure" to exist. Every driver
> just uses sysfs directly. I'm not sure that the best option --
> "input-like" infrastructure can make drivers even shorter -- but
> perhaps just directly using sysfs is best for simple task like a battery?
> 
> Jean, any ideas?

I guess we never felt any need for an "infrastructure", else we would
have created it. I have no idea what the "input infrastructure" looks
like so I can't compare. If you have something to propose which would
refactor some code amongst the hardware monitoring drivers or would
otherwise makes thing better, speak up :)

Note that the hwmon class is merely a way to find out which devices
have hardware monitoring attributes. There are no class attributes in
use. The reason is historical, and also due to the fact that no two
hardware monitoring chips have the same set of features.

-- 
Jean Delvare

Re: Generic battery interface

[Pavel Machek]

Hi!

> > Actually I do not see "hwmon infrastructure" to exist. Every driver
> > just uses sysfs directly. I'm not sure that the best option --
> > "input-like" infrastructure can make drivers even shorter -- but
> > perhaps just directly using sysfs is best for simple task like a battery?
> > 
> > Jean, any ideas?
> 
> I guess we never felt any need for an "infrastructure", else we would
> have created it. I have no idea what the "input infrastructure" looks
> like so I can't compare. If you have something to propose which would
> refactor some code amongst the hardware monitoring drivers or would
> otherwise makes thing better, speak up :)

I'm not sure if it is practical for hwmon, but having
report_voltage(x,y) is probably easier than coding sysfs stuff by
hand.
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: Generic battery interface

[Jean Delvare]

Hi Pavel,

> > I guess we never felt any need for an "infrastructure", else we would
> > have created it. I have no idea what the "input infrastructure" looks
> > like so I can't compare. If you have something to propose which would
> > refactor some code amongst the hardware monitoring drivers or would
> > otherwise makes thing better, speak up :)
> 
> I'm not sure if it is practical for hwmon, but having
> report_voltage(x,y) is probably easier than coding sysfs stuff by
> hand.

I can't figure out what it would look like in practice. Do you have
some code to show?

Thanks,
-- 
Jean Delvare

Re: Generic battery interface

[Greg KH]

> > 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.

I would tend to dispute this point :)

> Anyone volunteers write battery layer? If so, I'd go with /dev/XXX,
> otherwise I'd go with /sys/XXX, because it is simpler to code, and I
> believe it is also good enough...

Please go with /sys/battery/batteryN/XXXX, not an ioctl filled /dev/
interface that can not easily be scripted.

That way it is extensable (if the specific battery type just doesn't
provide a specific functionality, that sysfs file will not be present),
it allows for polling if needed, and would unify all the different types
of batteries.

Look at the kind of documentation the hwmon and i2c people have
created[1] for how to specify sysfs interfaces for different types of
sensor devices.  It should not be that hard to create the same for
something as "simple" as a battery :)

thanks,

greg k-h

[1] Documentation/hwmon/sysfs-interface

Re: Generic battery interface

[Vojtech Pavlik]

On Fri, Jul 28, 2006 at 12:16:32AM +0200, Pavel Machek wrote:


> /dev/XXX
> + battery layer will need to be invented
> - that layer will need maintainer
> + maintainer ensures this is not a mess, allocates numbers

  and figures out which features are identical, which not, etc. This
  	will actually be a lot easier than with keyboards: The number of
	different battery drivers/types is rather limited fortunately.

  note: It's absolutely necessary to limit the API to a well usable
	SUBSET of a superset of the features of all drivers/devices,
	even sacrificing obscure features to keep the API sane. One
	example would be the HID Power spec, which simply can't be
	supported to full extent by any sane API.

> - does not quite suit zaurus-style batteries, that can _only_ be
> 	polled. Can be worked-around by polling from kernel (and we
> 	are often doing that, anyway)

  + and the kernel can change the polling frequency based on power
	saving state changes

> + userspace backdoor interface will need to be invented, /dev/uinput style
> - hard to handle obscure features
> 	(do_not_charge_battery_for_X_minutes), and we'll probably want
> 	to unify batteries a bit, probably loosing precision.

-- 
Vojtech Pavlik
Director SuSE Labs

Re: Generic battery interface

[Shem Multinymous]

On 7/28/06, Vojtech Pavlik <vojtech@suse.cz> wrote:
>   note: It's absolutely necessary to limit the API to a well usable
>         SUBSET of a superset of the features of all drivers/devices,
>         even sacrificing obscure features to keep the API sane. One
>         example would be the HID Power spec, which simply can't be
>         supported to full extent by any sane API.

Non-standard functions must be handled reasonably within the
framework, otherwise drivers will have to build duplicate interfaces.

How about
  /sys/whatever/battery0/voltage for standard attributes
and
  /sys/whatever/battery0/thinkpad/inhibit-charge-minutes
for non-standard ones?


>   + and the kernel can change the polling frequency based on power
>         saving state changes

Likewise for cached attributes (query hardware only if N jiffies
passed since last querry, other return cached value). And that way,
hardware query frequency is never higher than what userspace actually
needs.

  Shem

Re: Generic battery interface

[Vojtech Pavlik]

On Fri, Jul 28, 2006 at 03:35:10AM +0300, Shem Multinymous wrote:
> On 7/28/06, Vojtech Pavlik <vojtech@suse.cz> wrote:
> >  note: It's absolutely necessary to limit the API to a well usable
> >        SUBSET of a superset of the features of all drivers/devices,
> >        even sacrificing obscure features to keep the API sane. One
> >        example would be the HID Power spec, which simply can't be
> >        supported to full extent by any sane API.
> 
> Non-standard functions must be handled reasonably within the
> framework, otherwise drivers will have to build duplicate interfaces.
> 
> How about
>  /sys/whatever/battery0/voltage for standard attributes
> and
>  /sys/whatever/battery0/thinkpad/inhibit-charge-minutes
> for non-standard ones?

I think the thinkpad features actually fall into the 'standard API', if
not for any other reason, then because some 80% of kernel developers use
ThinkPads. ;)

> >  + and the kernel can change the polling frequency based on power
> >        saving state changes
> 
> Likewise for cached attributes (query hardware only if N jiffies
> passed since last querry, other return cached value). And that way,
> hardware query frequency is never higher than what userspace actually
> needs.
 

-- 
Vojtech Pavlik
Director SuSE Labs

RE: Generic battery interface

[Brown, Len]

>On 7/28/06, Brown, Len <len.brown@intel.com> wrote:
>> I'm not religious about /dev vs. /sys.

Tho I'm starting to feel like I've touched off some religion in
others:-)

>I would greatly prefer a sysfs interface.

Understood.

>Having a clean, textual sysfs API that's easily accessed from shell
>has been extremely conductive for the development of the tp_smapi
>driver -- users can easily test and script the driver without extra
>programming and userspace components. Since tp_smapi is (AFAIK) the
>most feature-rich battery driver we now have, this is some to
>consider.

> clean

well, one man's "clean" is another man's "dirty", I guess this is
subjective.

> textual

good for shell scripts, not clear it is better for C programs
that have to open a bunch of files by name.

> sysfs was great for develping tp_smapi

Wonderful, but isn't the key here how simple it is for HAL
or X to understand and use the kernel API rather than the
developers of the kernel driver that implements the API?

If X were a shell script, I'd say a file per value would
clearly be the way to go, but it isn't.

-Len

Re: Generic battery interface

[Johan Vromans]

"Brown, Len" <len.brown@intel.com> writes:

> good for shell scripts, not clear it is better for C programs
> that have to open a bunch of files by name.

Moreover, it means that the factual data is only made available as a
nice textual string, that then has to be parsed by shell scripts and
other programs to get the factual data again.

So I feel much for /dev/xxx approach, together with the already
mentioned small utilities like 'batstat' for shell scripts.

-- Johan

Re: Generic battery interface

[Matthew Garrett]

On Fri, Jul 28, 2006 at 12:05:35AM -0400, Brown, Len wrote:

> Wonderful, but isn't the key here how simple it is for HAL
> or X to understand and use the kernel API rather than the
> developers of the kernel driver that implements the API?

HAL currently gets most of its information from sysfs, and managed to 
deal with parsing the existing /proc/acpi/battery stuff. I don't think 
there's any real difficulty there.

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

Re: Generic battery interface

[Vojtech Pavlik]

On Fri, Jul 28, 2006 at 10:58:06AM +0100, Matthew Garrett wrote:
> On Fri, Jul 28, 2006 at 12:05:35AM -0400, Brown, Len wrote:
> 
> > Wonderful, but isn't the key here how simple it is for HAL
> > or X to understand and use the kernel API rather than the
> > developers of the kernel driver that implements the API?
> 
> HAL currently gets most of its information from sysfs, and managed to 
> deal with parsing the existing /proc/acpi/battery stuff. I don't think 
> there's any real difficulty there.
 
Yes, HAL does a lot of ugly work to be able to gather the information it needs.

-- 
Vojtech Pavlik
Director SuSE Labs

Re: Generic battery interface

[Pavel Machek]

Hi!

> > textual
> 
> good for shell scripts, not clear it is better for C programs
> that have to open a bunch of files by name.
> 
> > sysfs was great for develping tp_smapi
> 
> Wonderful, but isn't the key here how simple it is for HAL
> or X to understand and use the kernel API rather than the
> developers of the kernel driver that implements the API?
> 
> If X were a shell script, I'd say a file per value would
> clearly be the way to go, but it isn't.

Well, lets say "if we go the /dev/XXX way, maintainer will probably
have to create small utility to make battery info available from the
shell".
									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/28/06, Brown, Len <len.brown@intel.com> wrote:
> good for shell scripts, not clear it is better for C programs
> that have to open a bunch of files by name.

> Wonderful, but isn't the key here how simple it is for HAL
> or X to understand and use the kernel API rather than the
> developers of the kernel driver that implements the API?

For a C program it's just open()+fscanf()+close(). You can easily wrap
it up in a 10-line function, and that's probably what HAL and friends
are already doing.

Anyway, I was just pointing out a practical advantage. The decision
about sysfs's textual interface has already been taken, for better or
worse, and I don't think it's good to invent a totally new interface
unless there's a strong technical reason why the sysfs model is
inappropriate for this task.

  Shem

RE: Generic battery interface

[Brown, Len]

>Anyone volunteers write battery layer? If so, I'd go with /dev/XXX,

I'd like to take a swing at it.
If it catches on, I'd be happy to maintain it.

I think we should be able to make different underlying battery
instrumentation make sense to user-space -- even Zaurus-style systems.

I'm not religious about /dev vs. /sys.  At the end of the day I think
that an easy and consistent programming I/F between user and kernel
is the highest priority, and at the moment for this type of thing
I think /dev is simpler than /proc or /sys files.  But if using
/dev has some fatal flaw, I'll be happy to change to /sys.
Also, there is no law that says we can't do some of both
if that turns out to be useful.

-Len

Re: Generic battery interface

[Shem Multinymous]

On 7/28/06, Brown, Len <len.brown@intel.com> wrote:
> I'm not religious about /dev vs. /sys.

I would greatly prefer a sysfs interface.
Having a clean, textual sysfs API that's easily accessed from shell
has been extremely conductive for the development of the tp_smapi
driver -- users can easily test and script the driver without extra
programming and userspace components. Since tp_smapi is (AFAIK) the
most feature-rich battery driver we now have, this is some to
consider.

(If you're not convinced yet, let me say one word: udev.)

  Shem

RE: Generic battery interface

[Brown, Len]

> ... need a consistent interface to common functionality.

Agreed.

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.

Re: battery events

Vojtech suggested that we create a virtual battery interface,
just like the input layer has virtual input devices.
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.

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.

With a /dev/battery0, and IOCTLS to get information from it,
the question becomes redundancy between that file and sysfs
text files.

Re: sysfs or /dev

It is tempting to claim that sysfs is extensible, but note
that whenever you create a sysfs file, you are creating an API,
which is a decision of exactly the same magnitude as defining
an IOCTL -- in either case, user-space needs to react to it
to understand it -- just that you can do that from the shell
with sysfs.

-Len

Re: Generic battery interface

[Shem Multinymous]

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.

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.

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.


> 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...


> 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.


  Shem

Re: Generic battery interface

[Daniel Barkalow]

On Thu, 27 Jul 2006, Shem Multinymous wrote:

> 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.

Maybe it would be best to have a virtual driver that knows about the union 
of the features, and makes whatever features are provided by the 
underlying driver available to userspace. That way, all of the drivers are 
implementing features out of a shared set, so you don't end up with 
thinkpad/force_discharge and something/discharge_battery. This is the 
principle behind, for example, the generic cdrom driver, which doesn't 
actually implement much but rather provides uniformity across devices 
handled by different drivers.

I don't think this is a case where each driver provides very similar 
functionality, but with critical differences which can't be corrected for; 
it seems like features are clearly available or not.

Of course, userspace can figure out which features are available by 
checking which files exist in the sysfs directory.

	-Daniel
*This .sig left intentionally blank*

Re: Generic battery interface

[Shem Multinymous]

On 7/27/06, Daniel Barkalow <barkalow@iabervon.org> wrote:
> Maybe it would be best to have a virtual driver that knows about the union
> of the features, and makes whatever features are provided by the
> underlying driver available to userspace. That way, all of the drivers are
> implementing features out of a shared set, so you don't end up with
> thinkpad/force_discharge and something/discharge_battery. This is the
> principle behind, for example, the generic cdrom driver, which doesn't
> actually implement much but rather provides uniformity across devices
> handled by different drivers.
>
> I don't think this is a case where each driver provides very similar
> functionality, but with critical differences which can't be corrected for;
> it seems like features are clearly available or not.

It's a bit trickier than it first looks.

For example, take the "force discharge" and "inhibit charge" commands.
Do they take effect until further notice, or for a given time period?
Whichever variant you pick, the ThinkPad hardware can't do it: you can
only tell it to force discharging until further notice, and you can
only tell it to avoid charging for N minutes. Note that the variants
cannot emulate each other, because when the laptop is suspended the
embedded controller still ticks but the kernel can't use timers to
issue new commands.

Another examle: suppose one hardware interface provide average power
reading while another interface provides only voltage and average
current. The two are not equivalent: if you try to compute power as
voltage*current you'll get inaccurate results, because power is the
integral over (instantaneous current)*(instantaneous voltage) and the
latter fluctuates. So you'd want to somehow prioritize the first
interface over the second, at least when it comes to power readings.

These details are inconsequential for most applications, but some do
care. So an interface that forces a certain view and hides the rest
may not be a good idea.

  Shem

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