Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhancements to Pico(W) gpio driver #874

Merged
merged 1 commit into from
Oct 29, 2023
Merged

Conversation

UncleGrumpy
Copy link
Collaborator

@UncleGrumpy UncleGrumpy commented Oct 10, 2023

Modifies the Pico gpio dirver to accept atoms for the special wl_gpio pins present in the Pico-W.

  • Adds support for {wl, 0..1} as output pins in gpio:digital_write/2.
  • Adds support for {wl, 2} as an input pin in gpio:digital_read/1.
  • Raises error badarg instead of returning error

These changes are made under both the "Apache 2.0" and the "GNU Lesser General
Public License 2.1 or later" license terms (dual license).

SPDX-License-Identifier: Apache-2.0 OR LGPL-2.1-or-later

Copy link
Collaborator

@pguyot pguyot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this match gpio module type?
I read pin() is a number or a tuple with an atom and a number.
Should we change the pin() type?

@UncleGrumpy
Copy link
Collaborator Author

Does this match gpio module type? I read pin() is a number or a tuple with an atom and a number. Should we change the pin() type?

Fixed now.

@UncleGrumpy UncleGrumpy reopened this Oct 24, 2023
@UncleGrumpy UncleGrumpy changed the title Allow using Pico-W WL_GPIO pins with gpio driver Enhancements to Pico(W) gpio driver Oct 26, 2023
@UncleGrumpy UncleGrumpy requested a review from fadushin October 26, 2023 22:32
@UncleGrumpy
Copy link
Collaborator Author

Does this match gpio module type? I read pin() is a number or a tuple with an atom and a number. Should we change the pin() type?

Fixed now.

Copy link
Collaborator

@pguyot pguyot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not convinced returning error tuples is the best semantic, but ERROR_ATOM was also wrong. I would prefer we raise badarg with bad arguments detected before making the platform API call (for example gpio_set_dir) and reserve error tuples for errors from the platform's API.

@UncleGrumpy
Copy link
Collaborator Author

We will need to rewrite the edoc for the driver if we are going to require try catch statement for some of the functions, otherwise deciphering the stack trace and looking up in the source where their code went wrong is a lot more work to decipher than an informative error return.

@pguyot
Copy link
Collaborator

pguyot commented Oct 27, 2023

This is Erlang, we don't need try / catch but just let the processes die 😁

@fadushin
Copy link
Collaborator

In general our pattern has been to raise an error on badarg (see the VALIDATE_VALUE macro, which we use extensively). As far as the docs, I don’t know if we need to fix every function with a @raises entry. OTP doesn’t go that far. And yeah, it’s Erlang, so “let it fail”.

@UncleGrumpy
Copy link
Collaborator Author

I guess I missed the point of #894.

@fadushin
Copy link
Collaborator

Here is an example from OTP:

6> try math:cos(srer) catch C:E:S -> {C, E, S} end. 
{error,badarg,
       [{math,cos,
              [srer],
              [{error_info,#{module => erl_stdlib_errors}}]},
        {erl_eval,do_apply,6,[{file,"erl_eval.erl"},{line,689}]},
        {erl_eval,try_clauses,8,[{file,"erl_eval.erl"},{line,919}]},
        {shell,exprs,7,[{file,"shell.erl"},{line,686}]},
        {shell,eval_exprs,7,[{file,"shell.erl"},{line,642}]},
        {shell,eval_loop,3,[{file,"shell.erl"},{line,627}]}]}

And here is the OTP doc: https://www.erlang.org/doc/man/math

(not pointing to OTP as the purveyors of perfect documentation, but…)

@fadushin
Copy link
Collaborator

fadushin commented Oct 27, 2023

I guess I missed the point of #894.

I think my point there was we need to change error to {error, Reason} in the API spec and behavior for the GPIO interface, which is a breaking API change (hence the new breaking label on the issue). We did something recently for the SPI and I2C interfaces. I am not sure if we looked at UART, as well, but we should.

As far as raising errors, exits, throws, etc (I think we use error), that is a separate issue, yes.

@UncleGrumpy
Copy link
Collaborator Author

UncleGrumpy commented Oct 27, 2023

But from this conversation it sounds like we want to raise bagarg not return error or {error, Reason}. Personally I think it is better to keep the behavior of the device divers consistent.

@fadushin
Copy link
Collaborator

fadushin commented Oct 27, 2023

But from this conversation it sounds like we want to raise bagarg not return error or {error, Reason}. Personally I think it is better to keep the behavior of the device divers consistent.

I think @pguyot ’s point (which I agree with) is that we raise errors on any parameter checks the user has supplied. These are almost always the equivalent of calling erlang:error(bardarg).

When we get to the point of implementing the logic of the function (e.g., interfacing with whatever system we are dealing with), and we get an error back from some API (usually an integer code, or something), we encode that error into something meaningful to the user, like an atom, or even a string if it makes sense (though we have to be careful with size, etc), and we return that error as a tuple: {error, Reason}.

I believe this is fairly established OTP behavior, if not AtomVM behavior.

The whole discussion of throw vs return is fraught with debate and not easy to untangle. I think even the idea of try-catch was not without controversy in OTP. Maybe one way to look at it is to distinguish between the kinds of error that are due to the programmer (bad args) and those that are due to the deployment or environment (case {error, Reason}) where the Reason can be logged, or even operated on, programmatically.

Copy link
Collaborator

@bettio bettio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about using the tuple semantic, so we can have {wl, 0} instead of creating new ad-hoc additions to the general semantic?

{ ATOM_STR("\x3", "wl0"), 0 },
{ ATOM_STR("\x3", "wl1"), 1 },
{ ATOM_STR("\x3", "wl2"), 2 },
SELECT_INT_DEFAULT(-1)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would use a mnemonic define here for -1.

@@ -96,7 +105,7 @@ static term nif_gpio_set_pin_mode(Context *ctx, int argc, term argv[])
int gpio_num = term_to_int(argv[0]);
int mode = interop_atom_term_select_int(pin_mode_table, argv[1], ctx->global);
if (UNLIKELY(mode < 0)) {
return ERROR_ATOM;
throw(BADARG_ATOM);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no throw in C. Use the RAISE_ERROR macro

term level_term = argv[1];

int level;
if (term_is_integer(level_term)) {
level = term_to_int32(level_term);
if (UNLIKELY((level != 0) && (level != 1))) {
return ERROR_ATOM;
throw(BADARG_ATOM);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RAISE_ERROR

@UncleGrumpy
Copy link
Collaborator Author

UncleGrumpy commented Oct 28, 2023

What about using the tuple semantic, so we can have {wl, 0} instead of creating new ad-hoc additions to the general semantic?

That is a very good point that I didn't even consider... I think that makes a lot more sense, I will go back and make that change.

@UncleGrumpy
Copy link
Collaborator Author

I pushed changes using a pin_tuple rather than atom, and cleaned up a few other places where VALIDATE_VALUE should have been used instead of if clauses.

@UncleGrumpy UncleGrumpy requested a review from bettio October 28, 2023 23:57
src/platforms/rp2040/src/lib/gpiodriver.c Outdated Show resolved Hide resolved
src/platforms/rp2040/src/lib/gpiodriver.c Outdated Show resolved Hide resolved
src/platforms/rp2040/src/lib/gpiodriver.c Show resolved Hide resolved
src/platforms/rp2040/src/lib/gpiodriver.c Outdated Show resolved Hide resolved
@pguyot
Copy link
Collaborator

pguyot commented Oct 29, 2023

I realize a lot of effort has been put in this PR and it is very good work. The pico gpio code was just a draft and the platform will soon have something on par with the others in this domain. Thank you @UncleGrumpy

@UncleGrumpy UncleGrumpy force-pushed the pico-w_gpio branch 3 times, most recently from 11668f6 to 66c8cdf Compare October 29, 2023 17:44
Modifies the Pico gpio dirver to accept atoms for the special wl_gpio pins
present in the Pico-W.
- Adds support for `wl0` and `wl1` as output pins in `gpio:digital_write/2`.
- Adds support for `wl2` as an input pin in `gpio:digital_read/1`.

Signed-off-by: Winford <[email protected]>
@bettio bettio changed the title Enhancements to Pico(W) gpio driver Enhancements to Pico(W) gpio driver Oct 29, 2023
@bettio bettio merged commit 7ee1adf into atomvm:master Oct 29, 2023
75 of 76 checks passed
@UncleGrumpy UncleGrumpy deleted the pico-w_gpio branch December 31, 2023 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants