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

samples: threads: update to use new GPIO API #21116

Merged
merged 1 commit into from
Dec 11, 2019

Conversation

mnkp
Copy link
Member

@mnkp mnkp commented Dec 3, 2019

Update sample application to use new GPIO API:

  • GPIO flags defined by the devicetree
  • replace gpio_pin_write with gpio_pin_set function

Update sample application to use new GPIO API:
- GPIO flags defined by the devicetree
- replace gpio_pin_write with gpio_pin_set function

Signed-off-by: Piotr Mienkowski <piotr.mienkowski@gmail.com>
Copy link
Member

@erwango erwango left a comment

Choose a reason for hiding this comment

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

Minor comment, otherwise I like it

__ASSERT_NO_MSG(gpio_dev != NULL);
gpio_dev = device_get_binding(led->gpio_dev_name);
__ASSERT(gpio_dev != NULL, "Error: didn't find %s device\n",
led->gpio_dev_name);
Copy link
Member

Choose a reason for hiding this comment

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

Would be more consistent sing printk, similarly as what is done below

Copy link
Member Author

Choose a reason for hiding this comment

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

The blinky and button samples use printk in this place so this is indeed not consistent. However, the ASSERT version is closer to what the real application would look like. Checking the result of device_get_binding is only required during the development phase. Once we provide a correct device name the check can be turned off. On the other hand the call to gpio_pin_configure can fail during the run time if we are working with an external GPIO driver.

I would rather stay with the existing approach.

Copy link
Member

Choose a reason for hiding this comment

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

The blinky and button samples use printk in this place so this is indeed not consistent. However, the ASSERT version is closer to what the real application would look like. Checking the result of device_get_binding is only required during the development phase. Once we provide a correct device name the check can be turned off.

This makes sense, but from what I see this isn't a widely used approach accros samples.
I'm approving the change, but it would be interesting to have this proposed (and potentially discussed) as a general guidelines for samples implementation.

__ASSERT_NO_MSG(gpio_dev != NULL);
gpio_dev = device_get_binding(led->gpio_dev_name);
__ASSERT(gpio_dev != NULL, "Error: didn't find %s device\n",
led->gpio_dev_name);
Copy link
Member

Choose a reason for hiding this comment

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

The blinky and button samples use printk in this place so this is indeed not consistent. However, the ASSERT version is closer to what the real application would look like. Checking the result of device_get_binding is only required during the development phase. Once we provide a correct device name the check can be turned off.

This makes sense, but from what I see this isn't a widely used approach accros samples.
I'm approving the change, but it would be interesting to have this proposed (and potentially discussed) as a general guidelines for samples implementation.

@galak galak merged commit 1d14b3b into zephyrproject-rtos:topic-gpio Dec 11, 2019
@mnkp mnkp deleted the samples_threads branch December 11, 2019 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants