[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> <port><pin> - 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 <input|set|clear|toggle|0|1> <port><pin>
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"
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> <port><pin> - 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
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> <port><pin> - 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.
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
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
Dear Detlev Zundel,
In message m2aacrlsen.fsf@ohwell.denx.de 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
Hi Wolfgang,
Dear Detlev Zundel,
In message m2aacrlsen.fsf@ohwell.denx.de 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
Am Donnerstag, den 07.07.2011, 12:15 +0200 schrieb Detlev Zundel:
Hi Wolfgang,
Dear Detlev Zundel,
In message m2aacrlsen.fsf@ohwell.denx.de 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:
- 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.
- Port output value
Read by "gpio value", as output latch is always readable.
- (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 <x> ; then ; else ; fi Of course, the same could be achieved with gpio value <x> ; 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 <input|set|1|clear|0|toggle|value> <port><pin> [env_var]
Another point: I'd like to quiet down the gpio command. Either in default case or via some other way (like gpio.q or another parameter). Call me picky, but I like my programs to be quiet unless there is an error. Especially when running scripted. Opinions/decisions ?
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
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 <op> value2\n" | " - set environment variable 'name' to the result of the evaluated\n" | " express specified by <op>. <op> can be &, |, ^, +, -, *, /, %\n" | " size argument is only meaningful if value1 and/or value2 are memory addresses" | ); `----
Cheers Detlev
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 <op> value2\n" | " - set environment variable 'name' to the result of the evaluated\n" | " express specified by <op>. <op> 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
participants (4)
-
Andreas Pretzsch -
Detlev Zundel -
Mike Frysinger -
Wolfgang Denk