#2390 assigned defect

GPIO API bug

Reported by: Ketul Shah Owned by: Needs Funding
Priority: normal Milestone: Indefinite
Component: unspecified Version: 4.10
Severity: normal Keywords:
Cc: Blocked By:
Blocking:

Description

Hello,

Recently GPIO API is merged with the mainline. And Eventually I used the same for GPIO driver for BBB. And while testing it on hardware I did not able to get the Pin Input value correctly. So after debugging I found that it is the issue with the function return type. A brief for the same is given :-

rtems_gpio_bsp_get_value() has return type of uint8_t. But as per API this function should return the bitmask. (DATAIN register & pinmask) . As BBB has pin no. 0 to 31 this should return 32 bits instead of 8.

Herewith I am attaching a patch for that.

Thanks.

Regards,
Ketul

Attachments (3)

gpio_api_bug.patch (1.9 KB) - added by Ketul Shah on Aug 13, 2015 at 2:22:34 PM.
0001-Fixes-ticket-2390-and-also-updates-the-RPI-implement.patch (3.5 KB) - added by André Marques on Aug 13, 2015 at 3:11:43 PM.
0001-Closes-ticket-2390-and-also-updates-the-RPI-implemen.patch (5.4 KB) - added by André Marques on Aug 17, 2015 at 10:50:44 AM.
Added ben and gedare suggestions.

Download all attachments as: .zip

Change History (11)

Changed on Aug 13, 2015 at 2:22:34 PM by Ketul Shah

Attachment: gpio_api_bug.patch added

comment:1 Changed on Aug 13, 2015 at 3:12:30 PM by André Marques

Although the BSP function rtems_gpio_bsp_get_value() may return the register value directly with the pin masked, the API function rtems_gpio_get_value() which is used by applications should still return 0 for logical low, 1 for logical high or -1 if the value could not be obtained. This allows for a more logical and constant behaviour of applications across multiple platforms/BSPS.

I have attached a new patch which updates the rtems_gpio_bsp_get_value doxygen documentation stating that a BSP may return 0xDEADBEEF if it could not get a value for the given pin, and also updates the raspberry pi implementation of the API to account for the rtems_gpio_bsp_get_value return type update.

comment:2 Changed on Aug 14, 2015 at 3:24:50 PM by Ben Gras

Personally I am not in favour of a magic constant such as 0xDEADBEEF. Who is to say hardware couldn't return such a magical value as a GPIO mask? Slightly nicer would be to at least define the constant in gpio.h as GPIO_IN_FAIL or something like that, so bsp's can use that constant to return it to gpio.c. I'd rather have the bsp functions return -1, 0 or 1 to indicate fail, low, high, but ok. That is my E0,02.

Either way I'm in favour of having this patch merged so the beagle bsp can merge its gpio implementation too. I think this also actually fixes bug with the invert logic feature I noticed while reading gpio.c..

comment:3 Changed on Aug 17, 2015 at 1:49:40 AM by Gedare

I also prefer -1, i.e. ~0 to indicate an error. It's an easy thing to check against.

Gedare

Changed on Aug 17, 2015 at 10:50:44 AM by André Marques

Added ben and gedare suggestions.

comment:4 Changed on Aug 17, 2015 at 12:27:37 PM by Ben Gras

Assuming the patch works as intended (LGTM) I'm in favour of merging it asap as it's blocking the beagle gpio merge.

comment:5 Changed on Aug 17, 2015 at 8:33:03 PM by Gedare

go ahead

comment:6 Changed on Aug 18, 2015 at 12:30:50 AM by Andre Marques <andre.lousa.marques@…>

In b09a578e8ad4583a97349f866e981980f0ff6ade/rtems:

Closes ticket #2390, and also updates the RPI implementation.

makes rtems_gpio_bsp_get_value return uint32_t. Motivation: simplify
beagle gpio implementation for common gpio apio.

comment:7 Changed on Jan 26, 2017 at 7:16:00 AM by Sebastian Huber

Milestone: 4.11.14.11.2

comment:8 Changed on Feb 15, 2017 at 1:37:51 PM by Sebastian Huber

Milestone: 4.11.2Indefinite
Owner: set to Needs Funding
Status: newassigned
Note: See TracTickets for help on using tickets.