Support to generic GPIOs class path names by bmeneg · Pull Request #523 · eclipse-mraa/mraa

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


    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

    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

        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. =)

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

Conversation

bmeneg commented Jun 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alext-mkrs commented Jun 7, 2016

Uh oh!

bmeneg commented Jun 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Hbrinj Jun 8, 2016

Choose a reason for hiding this comment

Uh oh!

alext-mkrs commented Jun 8, 2016

Uh oh!

bmeneg commented Jun 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alext-mkrs Jun 9, 2016

Choose a reason for hiding this comment

Uh oh!

bmeneg commented Jun 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alext-mkrs Jun 9, 2016

Choose a reason for hiding this comment

Uh oh!

alext-mkrs commented Jun 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alext-mkrs commented Jun 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bmeneg commented Jun 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alext-mkrs commented Jun 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bmeneg commented Jun 9, 2016

Uh oh!

arfoll commented Jun 9, 2016

Uh oh!

bmeneg commented Jun 14, 2016

Uh oh!

alext-mkrs Jun 18, 2016

Choose a reason for hiding this comment

Uh oh!

bmeneg Jun 22, 2016

Choose a reason for hiding this comment

Uh oh!

alext-mkrs Jun 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmeneg Jun 23, 2016

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

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