Implements getopt for avr #9174

virajsahai · 2018-05-24T01:35:02Z

Contribution description

Implemented getopt for atmega

Issues/PRs references

aabadie

Thanks for trying to tackle a quite old issue :)

I did a rough review of your change and you should reformat the files using uncrustify to be compliant with RIOT standards.
There are also other changes that are unrelated/not needed (mainly the removal of the stm32 header file).

aabadie · 2018-05-24T07:38:26Z

.gitattributes

@@ -5,3 +5,5 @@
 # when the heading is exactly 7 characters long.
 *.md conflict-marker-size=100
 *.txt conflict-marker-size=100
+*.c text eof=lf


This looks unrelated. Please provide those files in a separate PR if really required on your side.

I had to do this to get LF after every line. Please do let me know if there is any other way of getting rid of the trailing whitespace error. Thanks!

aabadie · 2018-05-24T07:39:29Z

cpu/atmega_common/getopt.c

+    char opt;
+    optarg = NULL;
+
+    if(optind < 1 || optind > argc)


Lot's of style issues, you can run uncrustify to format your files with a RIOT compliant style.

Will do! Thanks

aabadie · 2018-05-24T07:40:00Z

cpu/atmega_common/include/getopt.h

+extern "C" {
+#endif
+
+typedef struct{


This struct is not documented.

I am sorry for being extra lost but I am fairly new to Open Source. I went through all the dev recommendations but didn't find the location where to document it?

aabadie · 2018-05-24T07:40:10Z

cpu/atmega_common/include/getopt.h

+    int optopt;
+} opt_t;
+
+static const opt_t def_opt = {NULL,1,1,-1};


not documented

aabadie · 2018-05-24T07:40:54Z

cpu/atmega_common/include/getopt.h

+* @param[in]  argv           The array containing all the arguments.
+* @param[in]  optstring      The string of expected options.
+*
+* @return ASCII value of the option if option was succesfully found, '?' or ':' when unknown option is found or argument is missing.


This line is too long and should be split

Will do. Thanks!

aabadie · 2018-05-24T07:41:49Z

doc/doxygen/riot.doxyfile

@@ -826,6 +826,7 @@ EXCLUDE_PATTERNS       = */board/*/tools/* \
                         */cpu/msp430_common/include/sys/*.h \
                         */cpu/native/osx-libc-extra \
                         */cpu/x86/include/* \
+                         */cpu/atmega_common/include/getopt.h \


miri64 · 2018-05-24T10:09:04Z

Also, please provide a more telling commit message for your commit. It should tell what module got change and what the change entails (ideally <72 characters, at most 100). Our usual format would be something like

atmega_common: provide getopt support

Also nice to have (but not mandatory): some more detailed description about the change:

atmega_common: provide getopt support

libc-avr is lacking a `getopt` implementation. This change provides the
following feature set to provide basic `getopt` support

* ...

virajsahai · 2018-05-24T19:18:12Z

@aabadie Thanks for all the helpful comments. I have fixed most issues. just need to document the struct. Please let me know where and how, couldn't find the relevant doc. Thanks!!

aabadie · 2018-05-24T20:16:49Z

cpu/atmega_common/include/getopt.h

+extern "C" {
+#endif
+
+typedef struct {


To document this, just add a doxygen @brief before the struct. Each attribute must be document as well. See here for an example.

aabadie

Thanks for applying the changes fast @virajsahai.
I have found other minor things. See below.

Do you know a way for testing this ? Maybe @miri64 has an idea.

aabadie · 2018-05-24T20:17:42Z

cpu/atmega_common/getopt.c

+        }
+    }
+    return c;
+}


there's a missing newline at the end of this file.

aabadie · 2018-05-24T20:18:09Z

cpu/atmega_common/include/getopt.h

+ * @param[in]  argv           The array containing all the arguments.
+ * @param[in]  optstring      The string of expected options.
+ *
+ * @return ASCII value of the option if option was succesfully found.


typo on successfully

aabadie · 2018-05-24T20:18:16Z

cpu/atmega_common/include/getopt.h

+ * @param[in]  optstring      The string of expected options.
+ * @param[out] r              The per call struct for holding return args and other options.
+ *
+ * @return ASCII value of the option if option was succesfully found.


typo on successfully

aabadie · 2018-05-24T20:18:42Z

cpu/atmega_common/include/getopt.h

+ * @return ASCII value of the option if option was succesfully found.
+ *         '?' or ':' when unknown option is found or argument is missing.
+ */
+


this blank line can be removed

aabadie · 2018-05-24T20:18:50Z

cpu/atmega_common/include/getopt.h

+ * @return ASCII value of the option if option was succesfully found.
+ *         '?' or ':' when unknown option is found or argument is missing.
+ */
+


this blank line can be removed

virajsahai · 2018-05-24T22:12:48Z

@aabadie i have done some generic testing with some edge cases but haven't done the real deal. It'd be great if someone can advise on that. Thanks!

virajsahai · 2018-05-28T18:36:56Z

@miri64 @aabadie any updates??

aabadie

i have done some generic testing with some edge cases but haven't done the real deal. It'd be great if someone can advise on that.

What do you mean with "generic testing" ? I don't know either how this could be tested. Pinging @miri64, maybe you have a better idea ?

Otherwise, I have remaining style comments, mainly because of too long lines that can be split to fit into 80 characters length.

aabadie · 2018-05-28T19:46:38Z

cpu/atmega_common/getopt.c

+ * @file        getopt.c
+ * @brief       Implementation of getopt lib.
+ *
+ * This implements the getopt and getopt_r functionality. getopt_r has a few caveats, make sure to use it correctly.


This line should be split

In fact, it should be removed, it's a copy-paste of the one in the header file.

aabadie · 2018-05-28T19:46:45Z

cpu/atmega_common/getopt.c

+        return -1;
+    }
+
+    if (*argv[optind] != '-' || !(argv[optind] + 1) || *(argv[optind] + 1) == '\0' || *(argv[optind] + 1) == '-') {


This line should be split

aabadie · 2018-05-28T19:46:55Z

cpu/atmega_common/getopt.c

+        return -1;
+    }
+
+    if (*argv[r->optind] != '-' || !(argv[r->optind] + 1) || *(argv[r->optind] + 1) == '\0' || *(argv[r->optind] + 1) == '-') {


This line should be split

aabadie · 2018-05-28T19:47:56Z

cpu/atmega_common/include/getopt.h

+ * @file        header
+ * @brief       Implementation of getopt lib.
+ *
+ * This implements the getopt and getopt_r functionality. getopt_r has a few caveats, make sure to use it correctly.


This line should be split

aabadie · 2018-05-28T19:48:24Z

cpu/atmega_common/include/getopt.h

+/*INIT_OPT_T_PTR is used to initalize an opt_t* variable, INIT_OPT_T for opt_t. */
+/*To change options, edit the fields of opt_t and call getopt_r*/
+
+#define INIT_OPT_T_PTR &(opt_t){ NULL, 1, 1, -1 }


Those defines should be documented

miri64 · 2018-05-29T12:21:11Z

@aabadie i have done some generic testing with some edge cases but haven't done the real deal. It'd be great if someone can advise on that. Thanks!

Not sure why I specifically was summoned for this, but if you have test cases, please provide them as an application in tests/.

aabadie · 2018-05-29T14:43:44Z

Not sure why I specifically was summoned for this

because you opened #3355 ;)

virajsahai · 2018-05-29T21:43:09Z

@aabadie by generic testing, I meant testing the logic completeness in edge cases. I don't know how to test after integrating.

virajsahai · 2018-06-01T19:32:20Z

@aabadie any updates??

aabadie · 2018-06-01T19:40:20Z

I meant testing the logic completeness in edge cases

I don't understand this, sorry. What edge cases ? How do you do that ? Do you have a RIOT specific application for that ?

virajsahai · 2018-06-02T00:00:26Z

@aabadie No..i don't have a riot specific application for that. It would be great if you could point me to someone who can help with the testing.

And I tested the code as a regular c code on my pc..for cases when double colons are there..empty string passed etc.

virajsahai · 2018-06-11T23:31:36Z

@aabadie any updates?

aabadie · 2018-06-12T08:23:39Z

And I tested the code as a regular c code on my pc..for cases when double colons are there..empty string passed etc.

@virajsahai, I'm not sure but I think it would be nice to add a dedicated test application with shell and calls to the getopt function.
This way it will be possible to compare the versions provided by the different supported platforms. In this PR you provide a version for AVR, for ARM, I think the getopt from newlib is used. I don't know for MSP430 and others.
If getopt is not available for a given platform, it can be skipped using the BOARD_BLACKLIST environment variable in the Makefile.

It would be great to have other maintainers opinion on this. @gebart @kaspar030 @miri64 , what do you think ?

stale · 2019-08-10T06:08:05Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

miri64 · 2019-08-10T08:28:42Z

@jcarrano this somewhat coincides with your work in #9538. Can you have a look as well?

jcarrano · 2019-08-12T10:49:31Z

#9538 and this are not quite the same (#9538 does way more than getopt).

The question here is whether we seek to suport POSIX (or at least a reasonable subset). If we do, then regardless of #9538 this should go in.

stale · 2020-02-13T11:07:27Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

First getopt-atmega commit

d8a6703

virajsahai force-pushed the getopt-added-atmega branch from ad35c17 to d8a6703 Compare May 24, 2018 01:48

virajsahai changed the title ~~First getopt-atmega commit~~ Implements getopt for avr May 24, 2018

aabadie requested changes May 24, 2018

View reviewed changes

aabadie reviewed May 24, 2018

View reviewed changes

virajsahai force-pushed the getopt-added-atmega branch 2 times, most recently from fa96102 to f21e76a Compare May 24, 2018 22:10

aabadie requested changes May 28, 2018

View reviewed changes

virajsahai force-pushed the getopt-added-atmega branch from f21e76a to 7b25b9d Compare May 29, 2018 21:32

atmega_common: fixed issues in getopt implementation

c35577c

virajsahai force-pushed the getopt-added-atmega branch from 7b25b9d to c35577c Compare May 29, 2018 21:36

PeterKietzmann added the Platform: AVR Platform: This PR/issue effects AVR-based platforms label Jun 20, 2018

stale bot added the State: stale State: The issue / PR has no activity for >185 days label Aug 10, 2019

stale bot removed the State: stale State: The issue / PR has no activity for >185 days label Aug 10, 2019

stale bot added the State: stale State: The issue / PR has no activity for >185 days label Feb 13, 2020

stale bot closed this Mar 15, 2020

Implements getopt for avr #9174

Implements getopt for avr #9174

Conversation

virajsahai commented May 24, 2018

Contribution description

Issues/PRs references

aabadie left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

virajsahai May 24, 2018 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miri64 commented May 24, 2018 • edited Loading

virajsahai commented May 24, 2018

Choose a reason for hiding this comment

aabadie left a 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

Choose a reason for hiding this comment

virajsahai commented May 24, 2018 • edited Loading

virajsahai commented May 28, 2018

aabadie left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miri64 commented May 29, 2018

aabadie commented May 29, 2018

virajsahai commented May 29, 2018

virajsahai commented Jun 1, 2018

aabadie commented Jun 1, 2018

virajsahai commented Jun 2, 2018

virajsahai commented Jun 11, 2018

aabadie commented Jun 12, 2018

stale bot commented Aug 10, 2019

miri64 commented Aug 10, 2019

jcarrano commented Aug 12, 2019

stale bot commented Feb 13, 2020

aabadie left a comment •

edited

Loading

virajsahai May 24, 2018 •

edited

Loading

miri64 commented May 24, 2018 •

edited

Loading

virajsahai commented May 24, 2018 •

edited

Loading