add port for ATMega328P

[jonsnow1357]

No description provided.

[RichardBarry]

I note this is very similar to the ATMega323 port, the only difference being the constants used to access peripherals. For example, the ATMega323 uses TIMSK presumably because it only has one, whereas the ATMega328 uses TIMSK1 presumably because it has more than one TIMSK.

Would your x328 port also work for the x323 if the x323 were built with the latest WinAVR tools? That way we could just have one port. If not, could the port be generalised in some way so it works for both?

[jonsnow1357]

I have not checked the 323 datasheet, but different timers might be used.
I have tested mine on a Arduino board and I think Timer0 is used by the bootloader, so I went with Timer1
I can look into ATMega323 datasheet and come back with an answer

[jonsnow1357]

well, this might be more confusing than I intended ....

Most of ATmega UCs seem to have 3 timers: Timer0 (8-bit), Timer1 (16-bit) and Timer2 (8-bit)
However the registers controlling them are slightly different across the family. For example:

• ATmega32 has TIMSK only
• ATmega328P has TIMSK0, TIMSK1, TIMSK2. I suspect because there are 10 interrupt enable bits that do not fit into a 8 bit register ...
• ATmega128 has TIMSK and ETIMSK, Again because there seem to be more than 8 interrupt enable bits but they somehow decided to split it differently ...

Note 1
When I try to compile the existing port for ATmega323 I get the following error:
error: attempt to use poisoned "SIG_OUTPUT_COMPARE1A"
which according to the internet means this is the old name for the interrupt handler.
Also Atmega323 is obsolete and was replace with Atmega32/32L

Note 2:
If I try to compile my 328P port for 323, I get the expected error:
error: 'TIMSK1' undeclared (first use in this function)

Now, I am more of a HW designer and I am willing to help have AVR supported by FreeRTOS, but I don't think I know enough about the project to make SW architecture suggestions.
Pls, let me know how you want to proceed with this.

[cobusve]

I will take a look at this and come up with a suggestion over the next week somewhere.

[feilipu]

It may be useful to note that all modern ATmega devices use the same watchdog timer configuration, irrespective of their Timer characteristics. I exploited this characteristic to build an Arduino_FreeRTOS library that is completely agnostic^. So it may be a useful step to make a generic ATmega port using a WDT Tick rather than a proliferation of different Timer Tick versions.

Just also to note that all of the Timers are configured by the Arduino platform under the covers (eg. when using digitalRead()). So by using any of Timer 0, Timer 1, or Timer 2 for FreeRTOS with the Arduino IDE will break an Arduino feature (somewhere), or will cause FreeRTOS to cease to work.

^of course the ATmega2560 devices (3 byte PC) need to have a different portSAVE_CONTEXT(), and portRESTORE_CONTEXT() but the user doesn't need to see this at all.

[yuhui-zheng]

I compared port.c and portmacro.h in this PR with the ones under ./ATMega323 using this command:

FreeRTOS-Kernel/portable/GCC % diff -b ATMega323 ATMega328P

TL;DR -- Similar to Richard's original comment, is it possible to

1. have #ifdef #else #endif blocks around:

• #define portCOMPARE_MATCH_A_INTERRUPT_ENABLE ( ( uint8_t ) 0x10 )
• ucLowByte = TIMSK;
• TIMSK = ucLowByte;
  (Though the first one could theoretically use your implementation ( ( uint8_t ) (1 << OCIE1A) ) without #ifdef, for consistency I'd stick with values.)

2. update SIG_OUTPUT_COMPARE1A to TIMER1_COMPA_vect.

Open to suggestions.

Also ref -- "AVR084: Replacing ATmega323 by ATmega32" http://ww1.microchip.com/downloads/en/AppNotes/doc2518.pdf

---------------- Details ----------------
Since this PR is in open state for a while, and there are at least two other threads I'm aware of to port to ATmega328, I'm putting extra details in below. Between what's in this PR and what's in ATmega323, two sets of differences we could discuss in details.

1. Registers and bit mapping.
comparison result:

51,52c51,52
< #define portCLEAR_COUNTER_ON_MATCH				( ( uint8_t ) 0x08 )
< #define portPRESCALE_64							( ( uint8_t ) 0x03 )
---
> #define portCLEAR_COUNTER_ON_MATCH				( ( uint8_t ) (1 << WGM12) )
> #define portPRESCALE_64							( ( uint8_t ) ((1 << CS11) | (1 << CS10)) )
54c54
< #define portCOMPARE_MATCH_A_INTERRUPT_ENABLE	( ( uint8_t ) 0x10 )
---
> #define portCOMPARE_MATCH_A_INTERRUPT_ENABLE	( ( uint8_t ) (1 << OCIE1A) )
392c392
< 	ucLowByte = TIMSK;
---
> 	ucLowByte = TIMSK1;
394c394
< 	TIMSK = ucLowByte;
---
> 	TIMSK1 = ucLowByte;

From ATmega328P data sheet, about TCCR1B register

From ATmega323 data sheet, about TCCR1B register

So there's no functional change to portCLEAR_COUNTER_ON_MATCH and portPRESCALE_64

From ATmega328P data sheet, about TIMSK0/1/2 register

From ATmega323 data sheet, about TIMSK register

So the changes related to OCIE1A bit and TIMSK* register are necessary.

**2. Vector name. **
Comparison result:

405,406c405,406
< 	void SIG_OUTPUT_COMPARE1A( void ) __attribute__ ( ( signal, naked ) );
< 	void SIG_OUTPUT_COMPARE1A( void )
---
> 	void TIMER1_COMPA_vect( void ) __attribute__ ( ( signal, naked ) );
> 	void TIMER1_COMPA_vect( void )
418,419c418,419
< 	void SIG_OUTPUT_COMPARE1A( void ) __attribute__ ( ( signal ) );
< 	void SIG_OUTPUT_COMPARE1A( void )
---
> 	void TIMER1_COMPA_vect( void ) __attribute__ ( ( signal ) );
> 	void TIMER1_COMPA_vect( void )

https://www.nongnu.org/avr-libc/user-manual/group__avr__interrupts.html
It seems like TIMER1_COMPA_vect is the new vector name for both. I'm not an expert in AVR library, the link is for avr-libc version 2.0.0.

[yuhui-zheng]

@jonsnow1357 On a minor comment, apologies for the extra long review cycle... I attempted to rebase this PR, but it seems a lot of effort. If it's too much trouble, could open another PR and cherry-pick your two commits.

[feilipu]

If you’re going to go to the trouble of integrating the 328p into the mainline code, then it would make a lot more sense to add the 1284p and 2560 devices which have enough RAM to enable multiple useful threads to operate successfully.

The 328p simply doesn’t have enough RAM (2kB) to allow (for example) a network interface and a data logging device to operate in the same build. It is useful for an introduction to FreeRTOS, but not much else.

I’ve had another repository of AVR ports available for some years, and this is the port.c file, which covers 8u2, 16u2, 32u2, 16u4, 32u4, 168, 328p, 640, 1280, 1281, 1284p, and the two devices with 3 byte program counters 2560, 2561.

Any relevant timer, 8 bit (Timer 0), 16 bit (Timer 1, 2, 3), or Watch Dog Timer, can be used to trigger the System Tick. Timer controls are generalised depending on the MCU selected.

What would you like to achieve as an outcome from this PR?

You mention there are at least two other PRs around 328p. Are there any PRs open which cover all the (relevant) AVR platforms? Happy to contribute if that is seen as a better outcome?

[yuhui-zheng]

@feilipu

Thank you for the details.

To the most important question first:

What would you like to achieve as an outcome from this PR?

My thought is to make the port extensible enough while provide as much as we could. And the PRs could be incremental that it’s easier to review.

I’d propose getting 328P in first, and this would cover ATmega48A/PA/88A/PA/168A/PA/328/P data sheet. The RAM size of this series is 512/1K/1K/2KBytes, as you pointed out the use case may be very limited. In the following PRs, could further extend the port to cover ATmega640/V-1280/V-1281/V-2560/V-2561/V, ATmega164A/PA/324A/PA/644A/PA/1284/P, and … (And it’s very likely that less and less needs to be done to cover a new port.)

An assumption we made so far is that all the ATmega32 ports are based on the existing ATmega323, in which the port.c file only concerns tick timer related and task stack operation related. To carry out that assumption in the new ports, I think it would make sense to branch based on devices (e.g. in your code #if defined(__AVR_ATmega2560__)) at where timer is setup, and stack context is manipulated.

So in your [port.c],(https://github.com/feilipu/avrfreertos/blob/master/freeRTOS10xx/portable/port.c)

• you are saving 3 bytes for 2560/2561 as Program Counter is 17 bits wide, awesome.
• the timer setup is based on which timer user selects e.g. https://github.com/feilipu/avrfreertos/blob/master/freeRTOS10xx/portable/port.c#L695-L714, this may be less ideal. Since timer register bit mapping may change, even register names are the same. I'd suggest stick with branching based on device name, instead of register name. (Please see the example below for "register mapping got changed but register name stayed the same".)
• WDT and RTC. I would not put them in the same PR to speed up the review process. User could definitely select clock source, and it's nice to have these two provided for sure. I would put it aside as an improvement, not essential to the port.

From ATmega640/V-1280/V-1281/V-2560/V-2561/V data sheet --

bit 4 of TIMSK1 is OCIE1C.

From ATmega164A/PA/324A/PA/644A/PA/1284/P data sheet --

bit 4 of TIMSK1 is reserved.

Does not necessarily hurt, but I'd rather not make the assumption on behalf of chip vendor.

ps: I'm not sure if you and jonsnow are working together on this port. Above comment is based on "no". Throw PRs at us anytime. Could enable "allow edits from maintainer" when submitting, that I could rebase for you if needed.

[feilipu]

Generalised PR #48 done as requested. 👍

[yuhui-zheng]

@feilipu Thank you! Will review as soon as possible, and will post questions/feedback in #48.

@jonsnow1357 I may end up closing this PR, depends on how #48 works out. Apologies.

[jonsnow1357]

sorry, missed a couple of days to the exchange :(
no, @feilipu and me are not working together, my ATMega328P port is based on my trials to run FreeRTOS on a Arduino Nano. I agree it's not the greatest micro-controller due to limited resources, but since I had it working and you guys moved to github I thought it's worth a try to have this officially supported.

I will wait for #48 resolution ...
Best regards

[yuhui-zheng]

@jonsnow1357 I'd like to close this PR for now, since PR #48 seems to have a better coverage on ATmega family. Planning to merge #48 first, then having incremental changes there. If that interests you could play with the port and the simulator project linked one of the comments from me.

We'll try to have timely feedback moving forward... Thanks again for the contribution.