[PATCH] AVR EXT Topic is solved

[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.

[utzig]

Hi,

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

[tfAteba]

Hi Utzig,

This is new code.

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

[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

[tfAteba]

Hi,

Thank you for your feedback.

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

[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

[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!

[tfAteba]

Hi all,

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

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.

[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

[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.