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.

Sign up for GitHub

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

Jump to bottom

[topic-gpio] samples: reel_board: mesh_badge: Convert to the new GPIO API #22137

Merged
merged 2 commits into from
Jan 25, 2020
Merged

[topic-gpio] samples: reel_board: mesh_badge: Convert to the new GPIO API #22137

merged 2 commits into from
Jan 25, 2020

Conversation

carlescufi
Copy link
Member

See commit messages.

@carlescufi
Copy link
Member Author

Partially tested on the reel_board. @jhedberg could you please test it a bit more to make sure I haven't missed anything?

@zephyrbot
Copy link
Collaborator

zephyrbot commented Jan 23, 2020 •
edited
Loading

All checks are passing now.

checkpatch (informational only, not a failure)

-:191: WARNING:LONG_LINE: line over 80 characters
#191: FILE: samples/boards/reel_board/mesh_badge/src/reel_board.c:70:
+	{ .name = DT_ALIAS_LED0_GPIOS_CONTROLLER, .pin = DT_ALIAS_LED0_GPIOS_PIN,

-:193: WARNING:LONG_LINE: line over 80 characters
#193: FILE: samples/boards/reel_board/mesh_badge/src/reel_board.c:72:
+	{ .name = DT_ALIAS_LED1_GPIOS_CONTROLLER, .pin = DT_ALIAS_LED1_GPIOS_PIN,

-:195: WARNING:LONG_LINE: line over 80 characters
#195: FILE: samples/boards/reel_board/mesh_badge/src/reel_board.c:74:
+	{ .name = DT_ALIAS_LED2_GPIOS_CONTROLLER, .pin = DT_ALIAS_LED2_GPIOS_PIN,

- total: 0 errors, 3 warnings, 233 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

Your patch has style problems, please review.

NOTE: Ignored message types: AVOID_EXTERNS BRACES CONFIG_EXPERIMENTAL CONST_STRUCT DATE_TIME FILE_PATH_CHANGES MINMAX NETWORKING_BLOCK_COMMENT_STYLE PRINTK_WITHOUT_KERN_LEVEL SPLIT_STRING VOLATILE

NOTE: If any of the errors are false positives, please report
      them to the maintainers.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

@carlescufi carlescufi mentioned this pull request Jan 23, 2020
Closed
@carlescufi carlescufi force-pushed the mesh_badge-gpio-sample branch from 22b8877 to f0148fc Compare January 23, 2020 15:31
@carlescufi
boards: reel_board: Use the new GPIO flags in the DT source
71f5a88 
Use the flags included in the new GPIO API to describe the properties of
LEDs and buttons in the device tree source files.

Signed-off-by: Carles Cufi <carles.cufi@nordicsemi.no>
@carlescufi carlescufi force-pushed the mesh_badge-gpio-sample branch from f0148fc to 7fd50f8 Compare January 23, 2020 20:39
anangl
anangl requested changes Jan 24, 2020
View reviewed changes
samples/boards/reel_board/mesh_badge/src/reel_board.c Outdated
gpio_init_callback(&button_cb, button_interrupt, BIT(DT_ALIAS_SW0_GPIOS_PIN));
gpio_add_callback(gpio, &button_cb);
gpio_pin_interrupt_configure(gpio, DT_ALIAS_SW0_GPIOS_PIN,
GPIO_INT_EDGE_TO_ACTIVE);
Copy link
Member

@anangl anangl Jan 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

It should be GPIO_INT_EDGE_BOTH, see the definition of EDGE, which is no longer needed BTW.

Copy link
Member Author

Choose a reason for hiding this comment

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

Very well spotted. Fixed.

@carlescufi carlescufi force-pushed the mesh_badge-gpio-sample branch from 7fd50f8 to 6c83ce2 Compare January 24, 2020 13:09
@jhedberg
Copy link
Member

@jfischer-phytec-iot you might be interested to take a look as well

pabigot
pabigot requested changes Jan 24, 2020
View reviewed changes
Copy link
Collaborator

@pabigot pabigot left a comment

Choose a reason for hiding this comment

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

Only required change is handling an error return from gpio_pin_get(), which can be treated as "not pressed". Others are commentary.

samples/boards/reel_board/mesh_badge/src/reel_board.c Outdated
@@ -67,10 +65,14 @@ static struct {
struct device *dev;
const char *name;
u32_t pin;
Copy link
Collaborator

Choose a reason for hiding this comment

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

gpio_pin_t would save some space.

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, changed.

samples/boards/reel_board/mesh_badge/src/reel_board.c
{ .name = DT_ALIAS_LED0_GPIOS_CONTROLLER, .pin = DT_ALIAS_LED0_GPIOS_PIN, },
{ .name = DT_ALIAS_LED1_GPIOS_CONTROLLER, .pin = DT_ALIAS_LED1_GPIOS_PIN, },
{ .name = DT_ALIAS_LED2_GPIOS_CONTROLLER, .pin = DT_ALIAS_LED2_GPIOS_PIN, },
{ .name = DT_ALIAS_LED0_GPIOS_CONTROLLER, .pin = DT_ALIAS_LED0_GPIOS_PIN,
Copy link
Collaborator

Choose a reason for hiding this comment

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

You could just do DT_ALIAS_LED0_GPIOS which expands to the controller name, pin, and flags in a single initializer expression. Though without a pre-defined standard structure synchronized to generation of that expression it's technically more future-proof to spelling out each element.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, thanks for letting me know of this DT-generated macro. As you yourself mention, since this uses designated initializers I'd rather not change it at this point.

samples/boards/reel_board/mesh_badge/src/reel_board.c Outdated
gpio_pin_read(gpio, DT_ALIAS_SW0_GPIOS_PIN, &val);

return !val;
return gpio_pin_get(gpio, DT_ALIAS_SW0_GPIOS_PIN);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note gpio_pin_get can return a negative value to indicate a failure to read a value. There's also an essential type error here. gpio_pin_get(...) > 0 would be better.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed, fixed.

@carlescufi
samples: reel_board: mesh_badge: Convert to the new GPIO API
77ad2e8 
Convert to the new GPIO API using logical levels, and remove the
duplicate implementation of LED control that existed.

Signed-off-by: Carles Cufi <carles.cufi@nordicsemi.no>
@carlescufi carlescufi force-pushed the mesh_badge-gpio-sample branch from 6c83ce2 to 77ad2e8 Compare January 24, 2020 16:49
@carlescufi carlescufi changed the title samples: reel_board: mesh_badge: Convert to the new GPIO API [topic-gpio] samples: reel_board: mesh_badge: Convert to the new GPIO API Jan 25, 2020
@MaureenHelm MaureenHelm merged this pull request into zephyrproject-rtos:topic-gpio Jan 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mnkp mnkp mnkp approved these changes

@anangl anangl anangl approved these changes

@MaureenHelm MaureenHelm MaureenHelm approved these changes

@jhedberg jhedberg jhedberg approved these changes

@pabigot pabigot pabigot approved these changes

@galak galak Awaiting requested review from galak

@nashif nashif Awaiting requested review from nashif

@jfischer-no jfischer-no Awaiting requested review from jfischer-no

Assignees
No one assigned
Labels
area: Boards area: Samples Samples
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@carlescufi @zephyrbot @jhedberg @pabigot @MaureenHelm @anangl @mnkp