[PATCH] AVR EXT Topic is solved

ChibiOS public support forum for topics related to the Atmel AVR family of micro-controllers.

Moderators: utzig, tfAteba

User avatar
tfAteba
Posts: 547
Joined: Fri Oct 16, 2015 11:03 pm
Location: Strasbourg, France
Has thanked: 91 times
Been thanked: 48 times

[PATCH] AVR EXT

Postby tfAteba » Wed Oct 05, 2016 7:02 am

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.
Attachments
0001-ext-arduino-mega2560.patch.zip
(15.35 KiB) Downloaded 385 times
regards,

Theo.

utzig
Posts: 359
Joined: Sat Jan 07, 2012 6:22 pm
Location: Brazil
Has thanked: 1 time
Been thanked: 20 times
Contact:

Re: [PATCH] AVR EXT

Postby utzig » Mon Oct 17, 2016 1:15 pm

Hi,

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

User avatar
tfAteba
Posts: 547
Joined: Fri Oct 16, 2015 11:03 pm
Location: Strasbourg, France
Has thanked: 91 times
Been thanked: 48 times

Re: [PATCH] AVR EXT

Postby tfAteba » Mon Oct 17, 2016 7:28 pm

Hi Utzig,

This is new code.

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

Theo.

utzig
Posts: 359
Joined: Sat Jan 07, 2012 6:22 pm
Location: Brazil
Has thanked: 1 time
Been thanked: 20 times
Contact:

Re: [PATCH] AVR EXT

Postby utzig » Tue Oct 18, 2016 11:19 am

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

User avatar
tfAteba
Posts: 547
Joined: Fri Oct 16, 2015 11:03 pm
Location: Strasbourg, France
Has thanked: 91 times
Been thanked: 48 times

Re: [PATCH] AVR EXT

Postby tfAteba » Tue Oct 18, 2016 2:54 pm

Hi,

Thank you for your feedback.

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

Theo.

User avatar
Giovanni
Site Admin
Posts: 14444
Joined: Wed May 27, 2009 8:48 am
Location: Salerno, Italy
Has thanked: 1074 times
Been thanked: 921 times
Contact:

Re: [PATCH] AVR EXT

Postby Giovanni » Tue Oct 18, 2016 2:57 pm

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

User avatar
tfAteba
Posts: 547
Joined: Fri Oct 16, 2015 11:03 pm
Location: Strasbourg, France
Has thanked: 91 times
Been thanked: 48 times

Re: [PATCH] AVR EXT

Postby tfAteba » Tue Oct 18, 2016 3:08 pm

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!
regards,

Theo.

User avatar
tfAteba
Posts: 547
Joined: Fri Oct 16, 2015 11:03 pm
Location: Strasbourg, France
Has thanked: 91 times
Been thanked: 48 times

Re: [PATCH] AVR EXT

Postby tfAteba » Sun Oct 23, 2016 10:37 am

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.
Attachments
0002-ext-arduino-mega2560.patch.zip
(15.21 KiB) Downloaded 380 times
regards,

Theo.

utzig
Posts: 359
Joined: Sat Jan 07, 2012 6:22 pm
Location: Brazil
Has thanked: 1 time
Been thanked: 20 times
Contact:

Re: [PATCH] AVR EXT  Topic is solved

Postby utzig » Sun Oct 23, 2016 11:58 am

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

User avatar
tfAteba
Posts: 547
Joined: Fri Oct 16, 2015 11:03 pm
Location: Strasbourg, France
Has thanked: 91 times
Been thanked: 48 times

Re: [PATCH] AVR EXT

Postby tfAteba » Sun Oct 23, 2016 12:36 pm

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.
regards,

Theo.


Return to “AVR Support”

Who is online

Users browsing this forum: No registered users and 7 guests