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

Support to generic GPIOs class path names #523

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Support to generic GPIOs class path names #523

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions api/mraa/gpio.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,14 @@ int mraa_gpio_get_pin(mraa_gpio_context dev);
*/
int mraa_gpio_get_pin_raw(mraa_gpio_context dev);

/**
* Get the SYSFS pin path, invalid will return NULL
*
* @param dev The Gpio context
* @return Gpio pin sysfs path or return NULL if invalid
*/
char* mraa_gpio_get_path(mraa_gpio_context dev);

#ifdef __cplusplus
}
#endif
15 changes: 15 additions & 0 deletions api/mraa/gpio.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,21 @@ class Gpio
}
return mraa_gpio_get_pin(m_gpio);
}
/**
* Get pin SYSFS path.
*
* @throw std::runtime_error in case of failure
* @return Pin path string
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest to add "or NULL if error", to make it clear.

*/
std::string
getPath()
Copy link
Contributor

@arfoll arfoll Jun 21, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

in C++ this should return a std::string and throw exception if error

Copy link
Author

Choose a reason for hiding this comment

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

well pointed.

{
char* path = mraa_gpio_get_path(m_gpio);
if (path == NULL) {
throw std::logic_error("Failed to get GPIO path");
}
return path;
}

private:
mraa_gpio_context m_gpio;
Expand Down
1 change: 1 addition & 0 deletions include/mraa_internal_types.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ struct _gpio {
int pin; /**< the pin number, as known to the os. */
int phy_pin; /**< pin passed to clean init. -1 none and raw*/
int value_fp; /**< the file pointer to the value of the gpio */
char *gpio_path; /**< Custom file path format to help with compatibility */
void (* isr)(void *); /**< the interrupt service request */
void *isr_args; /**< args return when interrupt service request triggered */
pthread_t thread_id; /**< the isr handler thread id */
Expand Down
50 changes: 39 additions & 11 deletions src/gpio/gpio.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ static mraa_result_t
mraa_gpio_get_valfp(mraa_gpio_context dev)
{
char bu[MAX_SIZE];
sprintf(bu, SYSFS_CLASS_GPIO "/gpio%d/value", dev->pin);
sprintf(bu, SYSFS_CLASS_GPIO "%s/value", dev->gpio_path);
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO the original spacing was more natural as declaring bu and populating it were grouped together. Now populating is grouped with value_fp-related code, which is unrelated to bu

Copy link
Author

@bmeneg bmeneg Jun 9, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I usually don't consider semantics in this situation, I always separate declaration and its use/population, as others coding style standards mention (K&R for instance), but it's personal =P. I don't mind in revert it to the old way.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you do pure styling fixes keep them in a separate commit it makes reading much easier. Tbh when merging I try remove such things ;) in this case I'd say leave it unless u fix all the codebase to do that. If you're interested let's open a new issue on what and how we should fix and let's work out something

dev->value_fp = open(bu, O_RDWR);
if (dev->value_fp == -1) {
syslog(LOG_ERR, "gpio%i: Failed to open 'value': %s", dev->pin, strerror(errno));
Expand Down Expand Up @@ -73,6 +73,14 @@ mraa_gpio_init_internal(mraa_adv_func_t* func_table, int pin)
dev->advance_func = func_table;
dev->pin = pin;

// GPIO sysfs path may store the default path names schema or any other specific to vendor
dev->gpio_path = (char*) calloc(MAX_SIZE, sizeof(char));
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't this need to be freed in mraa deinit

if (dev->gpio_path == NULL) {
syslog(LOG_CRIT, "gpio%i: Failed to allocate memory for gpio_path attribute", pin);
status = MRAA_ERROR_INVALID_RESOURCE;
goto init_internal_cleanup;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

surplus styling fix (BTW doesn't it make it harder to read, by removing whitespace between logical blocks?)

if (IS_FUNC_DEFINED(dev, gpio_init_internal_replace)) {
status = dev->advance_func->gpio_init_internal_replace(dev, pin);
if (status == MRAA_SUCCESS)
Expand All @@ -95,11 +103,12 @@ mraa_gpio_init_internal(mraa_adv_func_t* func_table, int pin)
dev->isr_thread_terminating = 0;
dev->phy_pin = -1;

// then check to make sure the pin is exported.
char directory[MAX_SIZE];
snprintf(directory, MAX_SIZE, SYSFS_CLASS_GPIO "/gpio%d/", dev->pin);
snprintf(dev->gpio_path, MAX_SIZE, "/gpio%d", dev->pin);
snprintf(bu, sizeof(bu), SYSFS_CLASS_GPIO "%s", dev->gpio_path);
struct stat dir;
if (stat(directory, &dir) == 0 && S_ISDIR(dir.st_mode)) {

// check if the pin was already created, meaning another process may be the owner
if (stat(bu, &dir) == 0 && S_ISDIR(dir.st_mode)) {
dev->owner = 0; // Not Owner
} else {
int export = open(SYSFS_CLASS_GPIO "/export", O_WRONLY);
Expand All @@ -121,8 +130,14 @@ mraa_gpio_init_internal(mraa_adv_func_t* func_table, int pin)

init_internal_cleanup:
if (status != MRAA_SUCCESS) {
if (dev != NULL)
if (dev != NULL) {
if (dev->gpio_path != NULL) {
free(dev->gpio_path);
dev->gpio_path = NULL;
}
free(dev);
dev = NULL;
Copy link
Contributor

Choose a reason for hiding this comment

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

Surplus styling fix

}
return NULL;
}
return dev;
Expand Down Expand Up @@ -248,7 +263,7 @@ mraa_gpio_interrupt_handler(void* arg)
} else {
// open gpio value with open(3)
char bu[MAX_SIZE];
sprintf(bu, SYSFS_CLASS_GPIO "/gpio%d/value", dev->pin);
sprintf(bu, SYSFS_CLASS_GPIO "%s/value", dev->gpio_path);
fp = open(bu, O_RDONLY);
if (fp < 0) {
syslog(LOG_ERR, "gpio%i: interrupt_handler: failed to open 'value' : %s", dev->pin, strerror(errno));
Expand Down Expand Up @@ -337,7 +352,7 @@ mraa_gpio_edge_mode(mraa_gpio_context dev, mraa_gpio_edge_t mode)
}

char filepath[MAX_SIZE];
snprintf(filepath, MAX_SIZE, SYSFS_CLASS_GPIO "/gpio%d/edge", dev->pin);
snprintf(filepath, MAX_SIZE, SYSFS_CLASS_GPIO "%s/edge", dev->gpio_path);

int edge = open(filepath, O_RDWR);
if (edge == -1) {
Expand Down Expand Up @@ -479,7 +494,7 @@ mraa_gpio_mode(mraa_gpio_context dev, mraa_gpio_mode_t mode)
}

char filepath[MAX_SIZE];
snprintf(filepath, MAX_SIZE, SYSFS_CLASS_GPIO "/gpio%d/drive", dev->pin);
snprintf(filepath, MAX_SIZE, SYSFS_CLASS_GPIO "%s/drive", dev->gpio_path);

int drive = open(filepath, O_WRONLY);
if (drive == -1) {
Expand Down Expand Up @@ -542,7 +557,7 @@ mraa_gpio_dir(mraa_gpio_context dev, mraa_gpio_dir_t dir)
dev->value_fp = -1;
}
char filepath[MAX_SIZE];
snprintf(filepath, MAX_SIZE, SYSFS_CLASS_GPIO "/gpio%d/direction", dev->pin);
snprintf(filepath, MAX_SIZE, SYSFS_CLASS_GPIO "%s/direction", dev->gpio_path);

int direction = open(filepath, O_RDWR);

Expand Down Expand Up @@ -605,7 +620,7 @@ mraa_gpio_read_dir(mraa_gpio_context dev, mraa_gpio_dir_t *dir)
return MRAA_ERROR_INVALID_HANDLE;
}

snprintf(filepath, MAX_SIZE, SYSFS_CLASS_GPIO "/gpio%d/direction", dev->pin);
snprintf(filepath, MAX_SIZE, SYSFS_CLASS_GPIO "%s/direction", dev->gpio_path);
fd = open(filepath, O_RDONLY);
if (fd == -1) {
syslog(LOG_ERR, "gpio%i: read_dir: Failed to open 'direction' for reading: %s", dev->pin, strerror(errno));
Expand Down Expand Up @@ -766,7 +781,10 @@ mraa_gpio_close(mraa_gpio_context dev)
close(dev->value_fp);
}
mraa_gpio_unexport(dev);
free(dev->gpio_path);
dev->gpio_path = NULL;
free(dev);
dev = NULL;
return result;
}

Expand Down Expand Up @@ -817,3 +835,13 @@ mraa_gpio_get_pin_raw(mraa_gpio_context dev)
}
return dev->pin;
}

char*
mraa_gpio_get_path(mraa_gpio_context dev)
{
if (dev == NULL) {
syslog(LOG_ERR, "gpio: get_path: context is invalid");
return NULL;
}
return dev->gpio_path;
}
2 changes: 1 addition & 1 deletion tests/gpio_checks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ class GpioChecks(u.TestCase):

def setUp(self):
self.pin = m.Gpio(MRAA_GPIO)
self.gpio_path = "/sys/class/gpio/gpio" + str(self.pin.getPin(True))
self.gpio_path = "/sys/class/gpio" + self.pin.getPath()

def tearDown(self):
# dereference pin to force cleanup
Expand Down