[U-Boot] [RFC] gpio command: return value on write, additional actions
As of today (2011.06), the generic gpio command (common/cmd_gpio.c)
gpio
- input/set/clear/toggle the specified pin (e.g. PF10)
always returns the value read or set.
While this is sensible for read (input) and maybe (questionable) for
toggle, I don't see an usage for write (set/clear).
Also, this leads to unexpected side effects with complex constructs,
e.g. consider this environment:
setA=gpio set PF1
setB=gpio clear PF2
setAB_separate=env run setA ; env run setB
setAB_concatenated=env run setA setB
setBA_concatenated=env run setB setA
While executing "setAB_separate" and "setBA_concatenated" work as
expected, "setAB_concatenated" will only run setA, but not setB, as setA
"failed" (ret=1). [1]
The same would apply to e.g. && constructs.
As it works this way in the release and hence the behaviour is
essentially fixed, changing the return value is probably not an option.
OTOH, the breakage here might be negligible, at least for set/clear.
But in any case, it should be documented.
Beside this, I'd like to extend the gpio command with the additional
actions "0" and "1", always returning 0 (given the write was
successful):
gpio
As a nice benefit, it would strip down constructs like
if test ${LED_x} -eq 0;then gpio set PF1;else gpio clear PF1;fi
to a simple "gpio ${LED_x} PF1".
Any comments or objections before I submit a patch ?
[1] http://www.denx.de/wiki/DULG/CommandLineParsing "General rules"
--
carpe noctem engineering
Ingenieurbuero fuer Hard- & Software-Entwicklung Andreas Pretzsch
Dipl.-Ing. (FH) Andreas Pretzsch Tel. +49-(0)731-5521572
Hahnengasse 3 Fax: +49-(0)731-5521573
89073 Ulm, Germanyemail: a...@cn-eng.de
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [RFC] gpio command: return value on write, additional actions
On Tuesday, July 05, 2011 12:59:16 Andreas Pretzsch wrote: > As of today (2011.06), the generic gpio command (common/cmd_gpio.c) > gpio > - input/set/clear/toggle the specified pin (e.g. PF10) > always returns the value read or set. > > While this is sensible for read (input) and maybe (questionable) for > toggle, I don't see an usage for write (set/clear). the trouble with toggle is that there's no way to get the value without changing it to the input. we'd have to add another field that has no unintentional side effects like "gpio value". then we could change all the others to return 0/1 based on whether they succeeded, not based on the level of the gpio pin. > Also, this leads to unexpected side effects with complex constructs, > e.g. consider this environment: > setA=gpio set PF1 > setB=gpio clear PF2 > setAB_separate=env run setA ; env run setB > setAB_concatenated=env run setA setB > setBA_concatenated=env run setB setA > > While executing "setAB_separate" and "setBA_concatenated" work as > expected, "setAB_concatenated" will only run setA, but not setB, as setA > "failed" (ret=1). [1] > The same would apply to e.g. && constructs. ive never used the shell in u-boot, but couldnt the first logic be: setA=gpio set PF1 || : setB=gpio clear PF2 || : > As it works this way in the release and hence the behaviour is > essentially fixed, changing the return value is probably not an option. not really. poor behavior can be adjusted. -mike signature.asc Description: This is a digitally signed message part. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [RFC] gpio command: return value on write, additional actions
Am Dienstag, den 05.07.2011, 13:44 -0400 schrieb Mike Frysinger: > On Tuesday, July 05, 2011 12:59:16 Andreas Pretzsch wrote: > > As of today (2011.06), the generic gpio command (common/cmd_gpio.c) > > gpio > > - input/set/clear/toggle the specified pin (e.g. PF10) > > always returns the value read or set. > > > > While this is sensible for read (input) and maybe (questionable) for > > toggle, I don't see an usage for write (set/clear). > > the trouble with toggle is that there's no way to get the value without > changing it to the input. we'd have to add another field that has no True, at least on devices without hardware toggle support. And you never know if you read back the real pin state, the output latch or something invalid. And a "short" switch to input isn't always legal wrt to the attached hardware. Great when setting 1 bit is worth 10 bit of problems ;-) Personally, I'd shift the gpio_toggle to the underlying hardware specific code. And print an error if it's not supported. > unintentional side effects like "gpio value". then we could change all the > others to return 0/1 based on whether they succeeded, not based on the level > of the gpio pin. Didn't quite get that. In terms of "gpio value" = "give me the current (output latch) value without setting it to input if it's an output" ? We can't change the return value of "gpio input", as it's out in the wild and would break existing scripts. I'd say clear/set/toggle are changeable, don't see any legit return-value-usage here. For 100% backward compatibility, one could leave them as they are and use 0|1 as new actions with return 0, as proposed. So these variants: gpio clear|0 => set to output, write 0, return success gpio set|1 => set to output, write 1, return success gpio toggle => (set to output), toggle output, return success gpio input => set to input, return pin value gpio value => return current pin/latch/whatever value or gpio clear => set to output, write 0, return 0 gpio set => set to output, write 1, return 1 gpio 0 => set to output, write 0, return success gpio 1 => set to output, write 1, return success gpio toggle => (set to output), toggle output, return new_val gpio input => set to input, return pin value gpio value => return current pin/latch/whatever value Not perfectly symmetric, but the best way out I can think of. Maybe "get" instead of "value", as it's more usual. OTOH, get (to some people) implies "set to input", so value is more explicit. > > Also, this leads to unexpected side effects with complex constructs, > > e.g. consider this environment: > > setA=gpio set PF1 > > setB=gpio clear PF2 > > setAB_separate=env run setA ; env run setB > > setAB_concatenated=env run setA setB > > setBA_concatenated=env run setB setA > > > > While executing "setAB_separate" and "setBA_concatenated" work as > > expected, "setAB_concatenated" will only run setA, but not setB, as setA > > "failed" (ret=1). [1] > > The same would apply to e.g. && constructs. > > ive never used the shell in u-boot, but couldnt the first logic be: > setA=gpio set PF1 || : > setB=gpio clear PF2 || : Not fully, you'd need "gpio set PF1 || true". Not that this makes it less ugly... BTW, nobody would mind that without notice. Stumbled over it by pure accident myself. > > As it works this way in the release and hence the behaviour is > > essentially fixed, changing the return value is probably not an option. > > not really. poor behavior can be adjusted. > -mike Fine from my side, but I'm not the one to decide. -- carpe noctem engineering Ingenieurbuero fuer Hard- & Software-Entwicklung Andreas Pretzsch Dipl.-Ing. (FH) Andreas Pretzsch Tel. +49-(0)731-5521572 Hahnengasse 3 Fax: +49-(0)731-5521573 89073 Ulm, Germanyemail: a...@cn-eng.de ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [RFC] gpio command: return value on write, additional actions
On Tuesday, July 05, 2011 15:15:15 Andreas Pretzsch wrote: > Am Dienstag, den 05.07.2011, 13:44 -0400 schrieb Mike Frysinger: > > unintentional side effects like "gpio value". then we could change all > > the others to return 0/1 based on whether they succeeded, not based on > > the level of the gpio pin. > > Didn't quite get that. In terms of "gpio value" = "give me the current > (output latch) value without setting it to input if it's an output" ? yes. all it does is return gpio_get_value(). > We can't change the return value of "gpio input", as it's out in the > wild and would break existing scripts. i dont think this is that big of a deal > I'd say clear/set/toggle are changeable, don't see any legit > return-value-usage here. For 100% backward compatibility, one could > leave them as they are and use 0|1 as new actions with return 0, as > proposed. > > So these variants: > gpio clear|0 => set to output, write 0, return success > gpio set|1 => set to output, write 1, return success > gpio toggle => (set to output), toggle output, return success > gpio input => set to input, return pin value > gpio value => return current pin/latch/whatever value > or > gpio clear => set to output, write 0, return 0 > gpio set => set to output, write 1, return 1 > gpio 0 => set to output, write 0, return success > gpio 1 => set to output, write 1, return success > gpio toggle => (set to output), toggle output, return new_val > gpio input => set to input, return pin value > gpio value => return current pin/latch/whatever value > > Not perfectly symmetric, but the best way out I can think of. > Maybe "get" instead of "value", as it's more usual. OTOH, get (to some > people) implies "set to input", so value is more explicit. i prefer to have the command be simple and throw the extended logic onto the people writing scripts rather than trying to encode script logic into the commands themselves > > > Also, this leads to unexpected side effects with complex constructs, > > > > > > e.g. consider this environment: > > > setA=gpio set PF1 > > > setB=gpio clear PF2 > > > setAB_separate=env run setA ; env run setB > > > setAB_concatenated=env run setA setB > > > setBA_concatenated=env run setB setA > > > > > > While executing "setAB_separate" and "setBA_concatenated" work as > > > expected, "setAB_concatenated" will only run setA, but not setB, as > > > setA "failed" (ret=1). [1] > > > The same would apply to e.g. && constructs. > > > > ive never used the shell in u-boot, but couldnt the first logic be: > > setA=gpio set PF1 || : > > setB=gpio clear PF2 || : > > Not fully, you'd need "gpio set PF1 || true". Not that this makes it > less ugly... but it is doable if we want to just say now "ignore the value" without changing any code -mike signature.asc Description: This is a digitally signed message part. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [RFC] gpio command: return value on write, additional actions
Hi Andreas, > I'd say clear/set/toggle are changeable, don't see any legit > return-value-usage here. For 100% backward compatibility, one could > leave them as they are and use 0|1 as new actions with return 0, as > proposed. > > So these variants: > gpio clear|0 => set to output, write 0, return success > gpio set|1 => set to output, write 1, return success > gpio toggle => (set to output), toggle output, return success > gpio input => set to input, return pin value > gpio value => return current pin/latch/whatever value I'd propose to fix the commands to be sensible now. Actually I believe that they should not be in heavy use "in the wild" and so we should take the opportunity and declare the current behaviour as buggy and fix it. Rather now than later ;) Actually I would expect the "output" commands to return true when they were able to do what was requestes from them, i.e. drive the requested value to the output. I guess this cannot be done in the general case, but for a "weak output" that can be read back, this would be the most sensible thing to do. Cheers Detlev -- Helena ist verhältnismäßig leicht zu besetzen. Eine Frau, zarteste Jugend mit sinnlicher Reife verbindend; äußerst intelligent, indes von durchaus weiblicher Denkart; phlegmatisch, aber sensibel; unübertrefflich schön und dabei von sehr persönlichem Charme - mehr wird da nicht verlangt. -- Peter Hacks -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: d...@denx.de ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [RFC] gpio command: return value on write, additional actions
Dear Detlev Zundel, In message you wrote: > > > I'd say clear/set/toggle are changeable, don't see any legit > > return-value-usage here. For 100% backward compatibility, one could > > leave them as they are and use 0|1 as new actions with return 0, as > > proposed. Hm... I think it would be beneficial to bee able to get information about the current settings. For "clear" and "set" the result is known, but for "toggle" it would be helpful if we returned the current port state. Eventually we should add a "test" command (or "read" ?) that returns the current setting? > > So these variants: > > gpio clear|0 => set to output, write 0, return success > > gpio set|1 => set to output, write 1, return success > > gpio toggle => (set to output), toggle output, return success > > gpio input => set to input, return pin value > > gpio value => return current pin/latch/whatever value > > I'd propose to fix the commands to be sensible now. Actually I believe > that they should not be in heavy use "in the wild" and so we should take > the opportunity and declare the current behaviour as buggy and fix it. > Rather now than later ;) Agreed. > Actually I would expect the "output" commands to return true when they > were able to do what was requestes from them, i.e. drive the requested > value to the output. I guess this cannot be done in the general case, > but for a "weak output" that can be read back, this would be the most > sensible thing to do. For consistency I would prefer to have all commands return the same type of information, i. e. either an error status (like we do with all other commands - any result values would then have to be passed as environment settings), or we return the current port state (also for the "output" commands - see my comments for "toggle"). Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Hokey religions and ancient weapons are no substitute for a good blaster at your side. - Han Solo ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [RFC] gpio command: return value on write, additional actions
Hi Wolfgang, > Dear Detlev Zundel, > > In message you wrote: >> >> > I'd say clear/set/toggle are changeable, don't see any legit >> > return-value-usage here. For 100% backward compatibility, one could >> > leave them as they are and use 0|1 as new actions with return 0, as >> > proposed. > > Hm... I think it would be beneficial to bee able to get information > about the current settings. For "clear" and "set" the result is known, > but for "toggle" it would be helpful if we returned the current port > state. Eventually we should add a "test" command (or "read" ?) that > returns the current setting? In general the "current state" actually are three things: 1. Port Mode: Disabled (i.e. not connected to an external pin), Input, Output 2. Port output value 3. (Actually read-in) input value The current commands mix these aspects, i.e. they implicitely change the port mode. Differentiating between 2 and 3 will not always be possible but is - I've seen that on e.g. an i.mx31. >> > So these variants: >> > gpio clear|0 => set to output, write 0, return success >> > gpio set|1 => set to output, write 1, return success >> > gpio toggle => (set to output), toggle output, return success >> > gpio input => set to input, return pin value >> > gpio value => return current pin/latch/whatever value >> >> I'd propose to fix the commands to be sensible now. Actually I believe >> that they should not be in heavy use "in the wild" and so we should take >> the opportunity and declare the current behaviour as buggy and fix it. >> Rather now than later ;) > > Agreed. > >> Actually I would expect the "output" commands to return true when they >> were able to do what was requestes from them, i.e. drive the requested >> value to the output. I guess this cannot be done in the general case, >> but for a "weak output" that can be read back, this would be the most >> sensible thing to do. > > For consistency I would prefer to have all commands return the same > type of information, i. e. either an error status (like we do with all > other commands - any result values would then have to be passed as > environment settings), or we return the current port state (also for > the "output" commands - see my comments for "toggle"). Actually I would prefer to return an error status for all cases and return "valuable information" in the environment so we can easily use it subsequently. As we do not have a back-tick operator in the shell, I believe that return codes are only useful as error codes. Cheers Detlev -- ``The number of UNIX installations has grown to 10, with more expected.'' Unix Programmers Manual -- 1972 The number of UNIX variants has grown to dozens, with more expected. -- 2001 -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: d...@denx.de ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [RFC] gpio command: return value on write, additional actions
On Wednesday, July 06, 2011 06:36:00 Wolfgang Denk wrote: > For consistency I would prefer to have all commands return the same > type of information, i. e. either an error status (like we do with all > other commands - any result values would then have to be passed as > environment settings) going through the env seems a little wonky. i dont know of any other commands that do this, so i dont think there's any standard to work off of here ... -mike signature.asc Description: This is a digitally signed message part. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [RFC] gpio command: return value on write, additional actions
Hi Mike, > On Wednesday, July 06, 2011 06:36:00 Wolfgang Denk wrote: >> For consistency I would prefer to have all commands return the same >> type of information, i. e. either an error status (like we do with all >> other commands - any result values would then have to be passed as >> environment settings) > > going through the env seems a little wonky. i dont know of any other > commands > that do this, so i dont think there's any standard to work off of here ... I believe Wolfgang means for example this: ,[ common/cmd_setexpr.c ] | U_BOOT_CMD( | setexpr, 5, 0, do_setexpr, | "set environment variable as the result of eval expression", | "[.b, .w, .l] name value1 value2\n" | "- set environment variable 'name' to the result of the evaluated\n" | " express specified by . can be &, |, ^, +, -, *, /, %\n" | " size argument is only meaningful if value1 and/or value2 are memory addresses" | ); ` Cheers Detlev -- Als ich entführt wurde, begannen meine Eltern aktiv zu werden. Sie vermieteten mein Zimmer. -- Woody Allen -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: d...@denx.de ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [RFC] gpio command: return value on write, additional actions
On Fri, Jul 15, 2011 at 03:39, Detlev Zundel wrote: >> On Wednesday, July 06, 2011 06:36:00 Wolfgang Denk wrote: >>> For consistency I would prefer to have all commands return the same >>> type of information, i. e. either an error status (like we do with all >>> other commands - any result values would then have to be passed as >>> environment settings) >> >> going through the env seems a little wonky. i dont know of any other >> commands >> that do this, so i dont think there's any standard to work off of here ... > > I believe Wolfgang means for example this: > > ,[ common/cmd_setexpr.c ] > | U_BOOT_CMD( > | setexpr, 5, 0, do_setexpr, > | "set environment variable as the result of eval expression", > | "[.b, .w, .l] name value1 value2\n" > | " - set environment variable 'name' to the result of the > evaluated\n" > | " express specified by . can be &, |, ^, +, -, *, /, > %\n" > | " size argument is only meaningful if value1 and/or value2 are > memory addresses" > | ); > ` ah, this makes things much more clear, thanks -mike ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [RFC] gpio command: return value on write, additional actions
Am Donnerstag, den 07.07.2011, 12:15 +0200 schrieb Detlev Zundel: > Hi Wolfgang, > > > Dear Detlev Zundel, > > > > In message you wrote: > >> > >> > I'd say clear/set/toggle are changeable, don't see any legit > >> > return-value-usage here. For 100% backward compatibility, one could > >> > leave them as they are and use 0|1 as new actions with return 0, as > >> > proposed. > > > > Hm... I think it would be beneficial to bee able to get information > > about the current settings. For "clear" and "set" the result is known, > > but for "toggle" it would be helpful if we returned the current port > > state. Eventually we should add a "test" command (or "read" ?) that > > returns the current setting? Regarding toggle, the typical usage would be clocking or switch some LED or similar. In all these cases, the new value is irrelevant or could be read by the "get value" subcommand later. ( Which implies some "get output buffer" or "switch to input", depending on GPIO hardware structure. But this might be the case also for the low-level GPIO driver. ) All other "clever" direct usage of the new output value of toggle sounds to me like "write a driver" cases... So I'd opt for "return success", as with clear/0 and set/1. Regarding test/read, see below. > In general the "current state" actually are three things: > > 1. Port Mode: Disabled (i.e. not connected to an external pin), Input, >Output Already handled by the gpio_request() resp. gpio_direction_input() and gpio_direction_output() calls. At least, it should be this way, didn't check. > 2. Port output value Read by "gpio value", as output latch is always readable. > 3. (Actually read-in) input value Either via "gpio value" (read pin value or output latch, depending on hardware, essentially the return of gpio_get_value()) or via "gpio input", which would set the port to input and return its value. > The current commands mix these aspects, i.e. they implicitely change the > port mode. Differentiating between 2 and 3 will not always be possible > but is - I've seen that on e.g. an i.mx31. Most better GPIO blocks have a register both for output latch and for pin real value, as they have hardware toggle support. > >> > So these variants: > >> > gpio clear|0 => set to output, write 0, return success > >> > gpio set|1 => set to output, write 1, return success > >> > gpio toggle => (set to output), toggle output, return success > >> > gpio input => set to input, return pin value > >> > gpio value => return current pin/latch/whatever value As I understand, this proposal is still complete and valid. The only question (beside toggle return value) is the naming of the last command, either "value" or "get". I'd opt for value, as get implies for me (to some extent) switch to input. > >> I'd propose to fix the commands to be sensible now. Actually I believe > >> that they should not be in heavy use "in the wild" and so we should take > >> the opportunity and declare the current behaviour as buggy and fix it. > >> Rather now than later ;) > > > > Agreed. Great. So I'll go ahead and send a patch after we decided what exactly to do. > >> Actually I would expect the "output" commands to return true when they > >> were able to do what was requestes from them, i.e. drive the requested > >> value to the output. I guess this cannot be done in the general case, > >> but for a "weak output" that can be read back, this would be the most > >> sensible thing to do. Even with a decent GPIO module where you can read back the real pin value also in output mode, it's not always sufficient to read back directly (a few ns/us) after you set the output. For example, depending on the load attached to the output, it might take some time to charge to target level. Beside that, you demand from the low-level GPIO layer that it won't switch back to input by itself for a read. While it's a neat idea at the first sight, I think it's going too far for a script-used gpio command. > > For consistency I would prefer to have all commands return the same > > type of information, i. e. either an error status (like we do with all > > other commands - any result values would then have to be passed as > > environment settings), or we return the current port state (also for > > the "output" commands - see my comments for "toggle"). > > Actually I would prefer to return an error status for all cases and > return "valuable information" in the environment so we can easily use it > subsequently. As we do not have a back-tick operator in the shell, I > believe that return codes are only useful as error codes. True for now, beside one usage: if gpio value ; then ; else ; fi Of course, the same could be achieved with gpio value ; if test $? -eq 1 ; then ; else ; fi While we're at it, adding another optional parameter to store the value read in an environment variable sounds sensible: gpio [env_var] Another point: I'd like to quiet down the gpio comman
