Hi all!
I've just joined the list so hello all...
---
I have two items I'd like to throw open for discussion I'll start with the simplest wake on alarm
As you might have heard I've managed to get the Pandora to wake on alarm, however this requires a minor fix to the kernel http://bugs.openpandora.org/index.php?do=details&task_id=229
sorry I didn't keep the kernel code after I tested it as I borked it trying to do something else and deleted it in disgust!! Anyhow its a trivial "fix" I personally favour only leaving the alarm irq enabled if reg 0x2B (mask 0x08) is set I don't know leaving the alarm irq enabled all the time is a bad thing but its easy enough on shutdown to only enable it if needed...
---
My second item is about automagically shutting down the Pandora when its fully charged - and I know because of the feedback that is is a welcome facility for a number of people!
However I do have a fairly major concern about it, let me explain...
When I first wrote offWhenCharged.sh it directly manipulated the twl4030 registers using i2cset, for a few brief moments till paranoia set in I even released it in this state...
However having i2cset in a simple script in my view is *very* dangerous, there are no end of *really* dumb and potentially *dangerous* things someone could do "fiddling" with the values that i2cset "pokes" at the twl4030, if its there and easy to change people *will* mess with it!!!
I didn't and still don't see why OPL should pay for a single faulty machine caused by some muppet messing round with i2cset!
to this end I made a simple executable to which I have retained the source, this simply changes one setting or changes it back based on a command line parameter. (this is what offWhenCharged.sh calls)
I envisage in the future a simple executable that can change a number of different settings of the twl4030 in a muppet proof (tm) and safe way, if the source for this is in some are of the git repo, then its a little harder for someone to butcher it and break their Pandora...
Should OPL hold this source and only give it out to people who provide their order number and matching address so that they know who is experimenting with potentially damaging stuff ??
at some point I also want to turn offWhenCharged.sh into a zenity type thing that show's a window all the time its running, so the user is fully aware that a shut down is pending... (ideas thoughts welcome as I'd really like to see this end up in /usr/pandora/scripts....)
I don't favour a setting for this behaviour as someone might forget its set, start playing or worse doing some work while on charge and have it unexpectedly shut down!
phew! plenty enough for a first mail
Best Chris aka Chris_C
Hi,
On Wed, Apr 27, 2011 at 2:54 PM, Chris c.camacho@ntlworld.com wrote:
As you might have heard I've managed to get the Pandora to wake on alarm, however this requires a minor fix to the kernel http://bugs.openpandora.org/index.php?do=details&task_id=229
sorry I didn't keep the kernel code after I tested it as I borked it trying to do something else and deleted it in disgust!! Anyhow its a trivial "fix" I personally favour only leaving the alarm irq enabled if reg 0x2B (mask 0x08) is set I don't know leaving the alarm irq enabled all the time is a bad thing but its easy enough on shutdown to only enable it if needed...
Right, better only enable it when needed, i.e. when the alarm is set.
My second item is about automagically shutting down the Pandora when its fully charged - and I know because of the feedback that is is a welcome facility for a number of people!
However I do have a fairly major concern about it, let me explain...
When I first wrote offWhenCharged.sh it directly manipulated the twl4030 registers using i2cset, for a few brief moments till paranoia set in I even released it in this state...
Why do you need to directly manipulate twl4030 to shutdown pandora? Is that to prevent it from rebooting when plugged (is that really possible)?
However having i2cset in a simple script in my view is *very* dangerous, there are no end of *really* dumb and potentially *dangerous* things someone could do "fiddling" with the values that i2cset "pokes" at the twl4030, if its there and easy to change people *will* mess with it!!!
Right, and the proper way to do it is to have it all in the kernel. There is already a way to set alarm under Linux (/sys/class/rtc/rtc*/wakealarm) so the driver should be updated to handle that so that no direct i2c stuff is ever needed. If you really managed to prevent plugged pandora from resetting after shutdown (tell us how!), driver should be updated to always do that (it's what everyone is expecting anyway), and the script should just issue 'poweroff' command without ever touching i2c.
On Wed, 27 Apr 2011, Chris wrote:
# Should OPL hold this source and only give it out to people who provide their # order number and matching address so that they know who is experimenting with # potentially damaging stuff ??
Never; open source. At some point, you have to _trust_ developers.
What you're proposing is akin to welding shut the ghood of the car, since a few asshats will explode themselves. The reality is -- it is much better to let people tinker if they want to, and kill their machines if they see fit. Its not like the usual case is to brick the machine :)
More to point -- if a developer comes along and git's out the code and fuddles with it.. thats _GOOD_. Thast what we want -- we've got a handful of developers at all, lets get more, not lock them out :)
If a _user_ can brick themselves, this is entirely different.
Hence.. provide easy to use utilities or optional downloads, and keep the source open in the repo.
You know .. like the kernel :)
All the stuff you've had fun doing (and youv'e done some awesome hacks man :) is only doable because we encourage hacking; don't take it away from the next guy :)
jeff
Welcome! :) -- If everyone would put barbecue sauce on their food, there would be no war.
I wasn't proposing to take anything away from developers...
I surely am talking about idiots who know only a little and mess around not knowing what they are doing and then expecting a new machine gratis from OPL
A script having raw i2cset call *would* have cost OPL money - no doubt!
With any setting changed within the kernel, its a lot harder for a muppet to break somthing
No welding shut, just marking the steam hose "DANGER HOT - removable only with right handed star driver" - see the muppets don't have a right handed star driver - ie the ability to compile a kernel...
Best Chris_C
On 27/04/11 16:46, skeezix wrote:
On Wed, 27 Apr 2011, Chris wrote:
# Should OPL hold this source and only give it out to people who provide their # order number and matching address so that they know who is experimenting with # potentially damaging stuff ??
Never; open source. At some point, you have to _trust_ developers.
What you're proposing is akin to welding shut the ghood of the car, since a few asshats will explode themselves. The reality is -- it is much better to let people tinker if they want to, and kill their machines if they see fit. Its not like the usual case is to brick the machine :)
More to point -- if a developer comes along and git's out the code and fuddles with it.. thats _GOOD_. Thast what we want -- we've got a handful of developers at all, lets get more, not lock them out :)
If a _user_ can brick themselves, this is entirely different.
Hence.. provide easy to use utilities or optional downloads, and keep the source open in the repo.
You know .. like the kernel :)
All the stuff you've had fun doing (and youv'e done some awesome hacks man :) is only doable because we encourage hacking; don't take it away from the next guy :)
jeff
Welcome! :)
If everyone would put barbecue sauce on their food, there would be no war.
Hey guys, I've attached a diff of the drivers/power/twl4030_bci_battery.c I've been working on. It sets up the twl chip so it handles starting and stopping of the battery better, as per my charge hack that some people have been using here: http://boards.openpandora.org/index.php?/topic/1488-better-battery-charging-... It also detects when the battery is full, and instead of saying "Discharging" even though it is still plugged in, it now says "Full" in /sys/class/power_supply/twl4030_bci_battery/status. This can be used by scripts to detect when it is actually full. Related to Chris' work, it also handles the battery full interrupt in the twl4030battery_charged_evt function. I had thoughts to have it call a script in a known (root writable) location which could simply be Chris' shutdown interface (or any other logic we might want) but commented it out for three important reasons 1) I question the actual design safety of doing so. This is a script that would be run as root, and even though only root could edit that script, is that really a safe thing to do? 2) I have no idea how to do it. As you can see in the diff, I tried system(), but I can't just include stdlib.h. So if this is a good idea, someone please explain to me how a kernel module is supposed to execute a file? Actually, system or its ilk is probably an even worse idea, as it then blocks the interrupt. Gotta be a fork first. 3) I had a third reason but I've forgotten it now.
I want to incorporate Chris' "don't restart when shut down even if it's still plugged in" hack, but it really should be user selectable, which means dropping a new property into sysfs I think. Any thoughts? I've already got the show and set functions written, I just can't think of what property to use. Could we co-opt "CHARGE_FULL_DESIGN" for this? I can't find any information on what charge_full_design is supposed to be. May have to add our own custom properties for this, I think. Don't want to though, seems very unclean to me. Got to be a better way.
Anywho, definitely still to come are changes to pndevmapperd to monitor the battery status and make the charge LED blink when it reads Full. Probably tomorrow. Also, unless Chris gets to it first, I've got the changes to the rtc to allow the Pandora to be booted on alarm. It's a very simple change, just need to clean it up. Also probably tomorrow, or Friday.
once "full" even on main's isn't the battery discharging? afaict there is no way to only use power from AC and leave the battery alone - which is a shame (please tell me I'm wrong!)
Calling a userland script from the kernel is probably not a great idea.
I see nothing wrong with polling within a sleep loop as I maintain that halting should not be a setting but should only be initiated by something that a user has actively run and knows is going to happen /each/ time... (There should be a window saying something like "Awaiting shutdown on full charge <cancel>"
Yes leaving the alarm irq enabled is trivial isn't it ;) - can I leave you to provide the patch? As I mentioned earlier I don't have a kernel tree handy to play with and also my Pandora is going to have to go on holiday soon.... (to the health spa - damn that nub) :(
as for a sysfs property how about CHRISC_WANTS_TO_DISABLE_AC_TRANSITION :D or if you really must AC_POWER_UP_TRANSITION (true/false)
I don't see any issue with adding our own property name, what we want to accomplish will not be needed by phone OEM's and is specific to our needs...
besides isn't CHARGE_FULL_DESIGN to number of milliamps/hrs that the battery is designed to reach when brand new and charged?
Best Chris
On 28/04/11 04:14, David Goldsmith wrote:
Hey guys, I've attached a diff of the drivers/power/twl4030_bci_battery.c I've been working on. It sets up the twl chip so it handles starting and stopping of the battery better, as per my charge hack that some people have been using here: http://boards.openpandora.org/index.php?/topic/1488-better-battery-charging-... It also detects when the battery is full, and instead of saying "Discharging" even though it is still plugged in, it now says "Full" in /sys/class/power_supply/twl4030_bci_battery/status. This can be used by scripts to detect when it is actually full. Related to Chris' work, it also handles the battery full interrupt in the twl4030battery_charged_evt function. I had thoughts to have it call a script in a known (root writable) location which could simply be Chris' shutdown interface (or any other logic we might want) but commented it out for three important reasons
- I question the actual design safety of doing so. This is a script
that would be run as root, and even though only root could edit that script, is that really a safe thing to do? 2) I have no idea how to do it. As you can see in the diff, I tried system(), but I can't just include stdlib.h. So if this is a good idea, someone please explain to me how a kernel module is supposed to execute a file? Actually, system or its ilk is probably an even worse idea, as it then blocks the interrupt. Gotta be a fork first. 3) I had a third reason but I've forgotten it now.
I want to incorporate Chris' "don't restart when shut down even if it's still plugged in" hack, but it really should be user selectable, which means dropping a new property into sysfs I think. Any thoughts? I've already got the show and set functions written, I just can't think of what property to use. Could we co-opt "CHARGE_FULL_DESIGN" for this? I can't find any information on what charge_full_design is supposed to be. May have to add our own custom properties for this, I think. Don't want to though, seems very unclean to me. Got to be a better way.
Anywho, definitely still to come are changes to pndevmapperd to monitor the battery status and make the charge LED blink when it reads Full. Probably tomorrow. Also, unless Chris gets to it first, I've got the changes to the rtc to allow the Pandora to be booted on alarm. It's a very simple change, just need to clean it up. Also probably tomorrow, or Friday.
Firmware-dev mailing list Firmware-dev@openpandora.org http://openpandora.org/cgi-bin/mailman/listinfo/firmware-dev
On Thu, Apr 28, 2011 at 6:14 AM, David Goldsmith wizard@houseoffire.ca wrote:
- I question the actual design safety of doing so. This is a script
that would be run as root, and even though only root could edit that script, is that really a safe thing to do?
What are you really concerned about, the script doing i2c pokes or running a script in general? Running script using udev (see below) is standard way of doing this, cards are automounted when inserted this way for example.
- I have no idea how to do it. As you can see in the diff, I tried
system(), but I can't just include stdlib.h. So if this is a good idea, someone please explain to me how a kernel module is supposed to execute a file? Actually, system or its ilk is probably an even worse idea, as it then blocks the interrupt. Gotta be a fork first.
Proper way to call stuff from kernel is using udev uevents, power_supply_changed() should do that for you. Then you add udev rules that runs scripts or whatever.
However we already have skeezix's pndevmapperd monitoring the battery, so we can patch that to also look for battery full state and take some action.
I want to incorporate Chris' "don't restart when shut down even if it's still plugged in" hack, but it really should be user selectable, which means dropping a new property into sysfs I think. Any thoughts?
Yeah I don't think there is anything standard for this, so add a custom sysfs entry. Look how charge_current is done, using DEVICE_ATTR macro.
Also note that drivers/power/twl4030_bci_battery.c does not exist in 2.6.37 kernel, all this will have to be ported to drivers/power/twl4030_charger.c that exists there for the next firmware.