Support to generic GPIOs class path names #523

bmeneg · 2016-06-07T14:23:08Z

Older kernel versions might use gpio paths in the format of gpio<pin_number>_<pin_name> or any other way, as before device trees were supported proprietary hardware description files were passed from bootloader to kernel and hence leading to different naming convetions. For instance, on Allwinner platforms a specific .fex file was passed instead of the "Device Tree Binary" (.dtb).

Libmraa didn't support this kind of paths because all gpio operations were done with hard coded path names. With this commit it's possible to run libmraa either on newer kernel version and older ones, with both formats of gpio sysfs paths.

This PR also guarantees the kernel version discovery either when the gpio was already exported by any other process or when it is the first time used.

alext-mkrs · 2016-06-07T18:37:28Z

Haven't had a chance to analyze it in full yet, but what's immediately apparent is that it makes sense to squash both commits into one. And looks like you've removed a couple of "not owner" comments, which IMHO were helpful.

bmeneg · 2016-06-07T19:28:59Z

@alext-mkrs done, squashed both commits. But about the comments I removed: I added new comments a little bit more generic just before the 'if' block about this 'ownership', do you think would be better to revert it?

Now, about the Trevis CI failure state: I saw that on Travis CI the commits failed, but the failure report shows a problem to the test script itself:

$ sudo apt-get install -y -qq swig3.0 python git cmake

WARNING: The following packages cannot be authenticated!

  cmake cmake-data

E: There are problems and -y was used without --force-yes

The command "sudo apt-get install -y -qq swig3.0 python git cmake" failed and exited with 100 during .

I will commit the tests modification we discussed on Issue #520, I hope the CI passes then =).

Hbrinj · 2016-06-08T00:05:03Z

src/gpio/gpio.c

@@ -72,6 +78,7 @@ mraa_gpio_init_internal(mraa_adv_func_t* func_table, int pin)

    dev->advance_func = func_table;
    dev->pin = pin;
+    dev->gpio_path = (char*) calloc(MAX_SIZE, sizeof(char));


Won't this need to be freed in mraa deinit

alext-mkrs · 2016-06-08T10:20:38Z

Thanks for squashing, but now you have duplicated Signed-off-by line and two commit messages in one, I'd suggest just rephrase and merge them.

As for the comment - let me take a closer look on a computer in a day or two, as phone is not an ideal tool 😃

bmeneg · 2016-06-08T15:15:26Z

@Hbrinj well pointed. Indeed it's also needed to check its state after allocation and freed in case of error (goto init_internal_cleanup). Now it's done.

@alext-mkrs sorry for these wrong commits =), but now it's done, I squashed all commits together because all are related to the same thing. And don't worry, take a look when you have some time, phones isn't the better tool for sure.

alext-mkrs · 2016-06-09T17:00:20Z

include/mraa_internal_types.h

@@ -215,7 +216,7 @@ typedef struct {
    /*@{*/
    unsigned int pinmap; /**< sysfs pin */
    unsigned int parent_id; /** parent chip id */
-    unsigned int mux_total; /** Numfer of muxes needed for operation of pin */
+    unsigned int mux_total; /** Number of muxes needed for operation of pin */


Styling/typo fixes not directly related to the change at hand should better be split out into separate commits.

Othserwise they pollute git history and make it harder to git bisect/blame

bmeneg · 2016-06-09T18:21:09Z

Indeed, before we continue, I'll edit the Pull Request title, because I made a terrible mistake using the sentence "old kernel version", because this different GPIO class path name might be particularity to old kernel versions adopted by the Allwinner SoC vendor, which has its own internal things inside kernel translating the gpios to these schema. I'll change some things and come back later just with the dev->gpio_path generic attribute part, the path schema itself (gpio<pin_number>_<pin_name>) I'll handle inside Cubietruck's code, with gpio_init_internal_replace(dev, pin), and other platforms based on Allwinner might handle the same way. It's a better way, right?

alext-mkrs · 2016-06-09T18:21:29Z

src/gpio/gpio.c

@@ -136,7 +176,6 @@ mraa_gpio_init(int pin)
        syslog(LOG_ERR, "gpio%i: init: platform not initialised", pin);
        return NULL;
    }
-


Surplus styling fix

alext-mkrs · 2016-06-09T18:37:26Z

FWIW aside from those line comments I've added it looks ok to me by inspection. Haven't tried to run or explore it in runtime though.

alext-mkrs · 2016-06-09T18:48:40Z

<...> I'll change some things and come back later just with the dev->gpio_path generic attribute part, the path schema itself (gpio<pin_number>_<pin_name>) I'll handle inside Cubietruck's code, with gpio_init_internal_replace(dev, pin), and other platforms based on Allwinner might handle the same way. It's a better way, right?

Yes, I think so. That indeed changes the angle a bit 😃 If that's more of a platform-specific quirk, replace functions are there to help that.

and come back later just with the dev->gpio_path generic attribute part

I wonder if in this case this infrastructure is going to be useful anywhere else and if not - wouldn't using replace functions across the board for other GPIO functionalities (and adding the pin name part in those) be a better solution. Do you think that's feasible?

bmeneg · 2016-06-09T19:02:22Z

I wonder if in this case this infrastructure is going to be useful anywhere else and if not - wouldn't using replace functions across the board for other GPIO functionalities (and adding the pin name part in those) be a better solution. Do you think that's feasible?

Initially I thought it would be the solution, but there are hard coded path names are all around GPIO internal functions that doesn't have replaces. Then I though that the gpio_path attribute on gpio_context_t would help.

alext-mkrs · 2016-06-09T19:49:25Z

I see. You can try adding replaces though. Similar to what I did with pre- and post- (though replaces would be harder) in #515

Otherwise you could probably try the split way you mentioned (adding only path and the rest is in replace) and see what @arfoll has to say about that.

bmeneg · 2016-06-09T20:02:13Z

I'll try to use the gpio_path first, it's almost done :D, just change little things to clear the "Allwinner" specific parts. But if @arfoll prefers the replace functions, won't be a problem too. But thank you very much @alext-mkrs .. I'll be back soon.

arfoll · 2016-06-09T20:12:11Z

I think that by just concatenating either the pin num or the pin path we should be able to get both working relatively easily. I can't review well on my phone but I'll be back next week with a closer look :)

bmeneg · 2016-06-14T21:00:52Z

Well, I'm back with new changes, but now I didn't do any style fixes and everything specific to the gpio<pin_number>_<pin_name> were removed, because as I said before it was a specific case to Allwinner SoC based platforms using old kernel versions. The only thing I made in this commit was to add the gpio_path attribute to struct _gpio and handled it on gpio.c/.h and on the APIs to other languages.

Everything specific to Allwinner SoC must be handled in _replace functions on each platform support, as it's being made in the new support to Cubietruck I'm developing.

alext-mkrs · 2016-06-18T10:52:51Z

api/mraa/gpio.h

+ * Get the SYSFS pin path, invalid will return NULL
+ *
+ * @param dev The Gpio context
+ * @return Gpio pin sysfs path


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

but it already has "invalid will return NULL" in the same way other functions mention.

You refer to the comment line and I refer to the @return one. Adding it to @return will have it fully picked up by doxygen, meaning proper/better documenting.

Oh yeah, I understand and agree. =)

alext-mkrs · 2016-06-18T10:55:20Z

src/gpio/gpio.c

+    // 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));
+    if (dev->gpio_path == NULL) {
+        syslog(LOG_CRIT, "gpio%i: Failed to allocate memory for context's attribute", pin);


I'd still recommend to elaborate on "context's attribute" and just state directly it's the gpio_path we are talking about.

I didn't understand exactly what you mean by "context's attribute", could you give me an example?

What I mean is that instead of "gpio%i: Failed to allocate memory for context's attribute" make it a "gpio%i: Failed to allocate memory for gpio_path attribute" to make it clear, which exact piece were we not able to initialize.

Wow... I'm so bad on this xD. I'm sorry, you are completely right!

alext-mkrs · 2016-06-18T11:27:35Z

In addition to those inline comments - what's your idea about mraa_gpio_get_valfp(), where you haven't updated the /gpio%d/value piece? While it's used only behind _replace() checks in this file, I think it still would make sense to update it to use gpio_path for consistency in case there's a situation where there's no need for _replace(), but gpio_path is custom. And in case it's not custom gpio_path is gpio%d anyway, so we have backwards compatibility. What do you think?

arfoll · 2016-06-21T10:47:26Z

api/mraa/gpio.hpp

+     * @return Pin path string
+     */
+    char*
+    getPath()


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

well pointed.

arfoll · 2016-06-21T10:58:12Z

@bmeneguele can you push an example of a platform that works with this stuff as well? this seems to require a mix of _replace and gpio_path rather than just all moving to gpio_path. I'd much rather simply use gpio_path everywhere and have a small replace in the _init() that just sets gpio_path differently for platforms with names and by default set it to "gpio+dev->pin". Does that makes sense?

bmeneg · 2016-06-22T15:00:37Z

@alext-mkrs You are right.. I just forgot that peace, there should have the gpio_path as well.

@arfoll Yeah, I will open an PR to the cubietruck support I am working on. It isn't complete yet, I'm facing some SPI problems, but GPIO is working correctly. But as you can see on lines 108 and 109:

snprintf(dev->gpio_path, MAX_SIZE, "/gpio%d", dev->pin);
snprintf(bu, sizeof(bu), SYSFS_CLASS_GPIO "%s", dev->gpio_path);

the gpio_path is being set to gpio+dev->pin by default if there isn't any init_internal_replace() function on platform's _init() code.

bmeneg · 2016-06-22T21:06:47Z

@arfoll take a look at https://github.com/bmeneguele/mraa/commit/e93e261285586d4ec04c4758c541e93760cf8a7d this is the initial support to the Cubietruck board I'm working on right now. I won't pull request yet because I need to change the coding style to the standard of the project, but take a look at line 139 on src/arm/cubietruck.c, that is the _replace function to the gpio_init_internal function that handles the dev->gpio_path.

In some platforms using old Kernel versions uses different gpio class path names, like in all Allwinner SoC based boards before Kernel 4.0 device trees weren't used, and therefore gpio paths in sysfs were different from the actual standard. Libmraa doesn't supports this kind of paths because all gpio operations were done with hard coded path names following the gpios<pin_number> standard. With this commit it's possible to run libmraa either on newer kernel version and older ones, with any formats of gpio sysfs paths, anyone can specify an specific path standard. Also, the API to other languages was modified to support the new getPath() method which returns the current path being used. Signed-off-by: Bruno E. O. Meneguele <bmeneguele@gmail.com>

Hbrinj reviewed Jun 8, 2016
View reviewed changes

alext-mkrs reviewed Jun 9, 2016
View reviewed changes

bmeneg changed the title ~~Support to old fashion GPIOs class path names~~ Support to generic GPIOs class path names Jun 9, 2016

alext-mkrs reviewed Jun 18, 2016
View reviewed changes

arfoll reviewed Jun 21, 2016
View reviewed changes

arfoll force-pushed the master branch from 739861a to 3d790a8 Compare November 4, 2016 13:27

arfoll force-pushed the master branch from 0d7d9bc to 8ddbcde Compare May 12, 2017 13:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support to generic GPIOs class path names #523

Support to generic GPIOs class path names #523

bmeneg commented Jun 7, 2016 •

edited

Loading

alext-mkrs commented Jun 7, 2016

bmeneg commented Jun 7, 2016 •

edited

Loading

Hbrinj Jun 8, 2016

alext-mkrs commented Jun 8, 2016

bmeneg commented Jun 8, 2016 •

edited

Loading

alext-mkrs Jun 9, 2016

bmeneg commented Jun 9, 2016 •

edited

Loading

alext-mkrs Jun 9, 2016

alext-mkrs commented Jun 9, 2016 •

edited

Loading

alext-mkrs commented Jun 9, 2016 •

edited

Loading

bmeneg commented Jun 9, 2016 •

edited

Loading

alext-mkrs commented Jun 9, 2016 •

edited

Loading

bmeneg commented Jun 9, 2016

arfoll commented Jun 9, 2016

bmeneg commented Jun 14, 2016

alext-mkrs Jun 18, 2016

bmeneg Jun 22, 2016

alext-mkrs Jun 23, 2016 •

edited

Loading

bmeneg Jun 23, 2016

alext-mkrs Jun 18, 2016

bmeneg Jun 22, 2016

alext-mkrs Jun 23, 2016

bmeneg Jun 23, 2016

alext-mkrs commented Jun 18, 2016

arfoll Jun 21, 2016 •

edited

Loading

bmeneg Jun 22, 2016

arfoll commented Jun 21, 2016

bmeneg commented Jun 22, 2016 •

edited

Loading

bmeneg commented Jun 22, 2016

Support to generic GPIOs class path names #523

Are you sure you want to change the base?

Support to generic GPIOs class path names #523

Conversation

bmeneg commented Jun 7, 2016 • edited Loading

alext-mkrs commented Jun 7, 2016

bmeneg commented Jun 7, 2016 • edited Loading

Choose a reason for hiding this comment

alext-mkrs commented Jun 8, 2016

bmeneg commented Jun 8, 2016 • edited Loading

Choose a reason for hiding this comment

bmeneg commented Jun 9, 2016 • edited Loading

Choose a reason for hiding this comment

alext-mkrs commented Jun 9, 2016 • edited Loading

alext-mkrs commented Jun 9, 2016 • edited Loading

bmeneg commented Jun 9, 2016 • edited Loading

alext-mkrs commented Jun 9, 2016 • edited Loading

bmeneg commented Jun 9, 2016

arfoll commented Jun 9, 2016

bmeneg commented Jun 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alext-mkrs Jun 23, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alext-mkrs commented Jun 18, 2016

arfoll Jun 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arfoll commented Jun 21, 2016

bmeneg commented Jun 22, 2016 • edited Loading

bmeneg commented Jun 22, 2016

bmeneg commented Jun 7, 2016 •

edited

Loading

bmeneg commented Jun 7, 2016 •

edited

Loading

bmeneg commented Jun 8, 2016 •

edited

Loading

bmeneg commented Jun 9, 2016 •

edited

Loading

alext-mkrs commented Jun 9, 2016 •

edited

Loading

alext-mkrs commented Jun 9, 2016 •

edited

Loading

bmeneg commented Jun 9, 2016 •

edited

Loading

alext-mkrs commented Jun 9, 2016 •

edited

Loading

alext-mkrs Jun 23, 2016 •

edited

Loading

arfoll Jun 21, 2016 •

edited

Loading

bmeneg commented Jun 22, 2016 •

edited

Loading