Page 1 of 9

[PATCH] AVR EXT

Posted: Wed Oct 05, 2016 7:02 am
by tfAteba
Here is a patch to implement EXT for arduino mega2560.

The implementation was made with chibios-svn-9835-trunk.

If you apply the patch to a trunk tree of ChibiOS, the following will be done:
- Modify some file (os/hal/include/hal_ext.h and os/hal/ports/AVR/platform.mk)
- Creation of the EXT driver low device driver in os/hal/ports/AVR
- Creation of a test directory under testhal/AVR/. This is an application to test the driver.

A lot of optimisations can be done maybe?
If there is a better way to do things don't hesitated to share!

It will be nice to have some feedback on the code style and the application execution.

I will be happy to improve if nedded.

Giovanni, Utzig, I also need your help to see if driver design rules are respected so that this can be part of the AVR port?

Any feedback are welcome.

Re: [PATCH] AVR EXT

Posted: Mon Oct 17, 2016 1:15 pm
by utzig
Hi,

Is this new code or is it based on the changes Igor had written on his github repo?

Re: [PATCH] AVR EXT

Posted: Mon Oct 17, 2016 7:28 pm
by tfAteba
Hi Utzig,

This is new code.

To create it I looked at STM32 EXT driver and base my work in the old manner.

Re: [PATCH] AVR EXT

Posted: Tue Oct 18, 2016 11:19 am
by utzig
In general the patch looks ok. If you could format it to follow the ChibiOS/RT style that would be helpful, specifically never using tabs, using two spaces for indentation, one space after keywords like if and while and the open parenthesis, one space after the opening { at line ends.

Also you should not use sei and cli but instead some form of osalSysLock/Unlock*.

And also you can attribute the copyright to yourself if you would prefer that!

Cheers,
Fabio Utzig

Re: [PATCH] AVR EXT

Posted: Tue Oct 18, 2016 2:54 pm
by tfAteba
Hi,

Thank you for your feedback.

I will review the patch and improve it according your advices.

Re: [PATCH] AVR EXT

Posted: Tue Oct 18, 2016 2:57 pm
by Giovanni
Correct, everything HAL-related can be under copyright of the authors as long the Apache 2.0 license is kept (in order to have a single license to deal with).

Another note, templates files are under my copyright but that can be changed, probably we should remove my name there and just leave an XXXXXXXX inside the license text.

I also "strongly encourage" to take coding rules seriously ;)

Giovanni

Re: [PATCH] AVR EXT

Posted: Tue Oct 18, 2016 3:08 pm
by tfAteba
Thank you for your prescision.

Note that for the code style I was not aware of.

I will submit another patch to see if every things is correct!

Re: [PATCH] AVR EXT

Posted: Sun Oct 23, 2016 10:37 am
by tfAteba
Hi all,

Utzig, here is the patch formated by following ChibiOS style :D .

I have also attribute the copyright to myself for the following three files:
    - os/hal/ports/AVR/hal_ext_lld.c
    - os/hal/ports/AVR/hal_ext_lld.h
    - testhal/AVR/EXT/main.c

Let me know is there is still another important point to review.

Re: [PATCH] AVR EXT  Topic is solved

Posted: Sun Oct 23, 2016 11:58 am
by utzig
Thanks, applied!

Btw, you added a new define for a new ext mode

Code: Select all

#define EXT_CH_MODE_LOW_LEVEL       5U  /**< @brief low level callback.     */
and ended up not using it. I would suppose it's the else condition in ext_lld_set_intx_edges. I left it there anyway.

Cheers,
Fabio Utzig

Re: [PATCH] AVR EXT

Posted: Sun Oct 23, 2016 12:36 pm
by tfAteba
The Mega has 4 trigger modes, falling, rising, both and low level mode.

The else if of course for the low level mode(EXT_CH_MODE_LOW_LEVEL). May be I should use it explicitly, just want to know for next time.

Thanks.