Hi KR,
Sorry for my delay to review your commits.
First kudos for your first contribution, it passed without any issues in
the checkpatch:
$ ./tools/checkpatch.sh -g HEAD~...HEAD
$
You did really great including all necessary information in the commit log
messages:
commit 9ce4bbf808416b9c5be2347c4a19b667c676db48 (HEAD -> master)
Author: Kerogit <kr....@kerogit.eu>
Date: Sat Apr 5 23:41:54 2025 +0200
arch/avr: fixed PC load on chips with more than 128kB program memory
The code used subi instruction to decrement the Y pointer but this
instruction only affects a byte, not a word. Fixed it by using
pre-decrement in the LD instruction instead.
commit b03272ff3d316352a7b735d6325dd0a2bcadb924
Author: Kerogit <kr....@kerogit.eu>
Date: Mon Mar 24 22:43:33 2025 +0100
nuttx/compiler: replace AVR __flash qualifier with __memx for some chips
The __flash qualifier only allows access to program memory below 64kB
mark (uses 16 bit pointer.) Since many chips currently supported
have more than that, this qualifier can prove insufficient.
When flagged __memx the compiler will use 24 bit pointer to access
the variable.
commit b3831188fb75b4b13302bf00c37e66e0155ce0cb
Author: Kerogit <kr....@kerogit.eu>
Date: Mon Mar 24 22:17:09 2025 +0100
arch/avr/atmega: flag microcontrollers that have RAMPZ register
Define preprocesor macros that trigger adding RAMPZ register
into thread context
commit 20883f7ea9f8050f2c51e8067ebfb0fffb621e54
Author: Kerogit <kr....@kerogit.eu>
Date: Mon Mar 17 12:20:35 2025 +0100
arch/avr: save RAMPZ register into thread context
On chips that have it, RAMPZ is part of the thread context
and needs to be saved as well
commit 00d803331e8aeb5f08dc09eafa0980aff8b83e7e
Author: Kerogit <kr....@kerogit.eu>
Date: Thu Mar 13 20:44:01 2025 +0100
arch/avr: fixed typo in Kconfig
commit a2977a1fd20ddc9a596e289debaebdfd06685328
Author: Kerogit <kr....@kerogit.eu>
Date: Mon Apr 7 20:43:13 2025 +0200
Documentation/reference/user/02_task_scheduling: fixed function name
The document referred to sched_get_rr_interval function but only
sched_rr_get_interval exists in the sources.
You just missed to signed your commits (git commit -S)
I didn't understand these two configs:
config AVR_HAS_RAMPZ
bool
depends on AVR_ATMEGA_HAS_RAMPZ
config AVR_ATMEGA_HAS_RAMPZ
bool
select AVR_HAS_RAMPZ
They seem like cross-references.
Why don't you just use AVR_HAS_RAMPZ in Atmega chips directly?
I suggest you open the PR in the github to integrate your contribution into
the mainline, please read the
https://nuttx.apache.org/docs/latest/contributing/index.html for
instructions.
BR,
Alan
On Thu, Apr 10, 2025 at 5:54 AM <kr....@kerogit.eu> wrote:
> Hello,
>
> I would like to solicit a review for some fixes of (mostly) AVR
> architecture code. These are some things I noticed when attempting to
> create a port for AVR DA/DB family. I am doing my development based on
> ATmega family since I am familiar with that and these patches (should)
> only apply to it. I tried to avoid any changes that would affect the
> other families but I cannot do any testing to confirm these patches
> don't break anything for them.
>
> I tested these patches with ATmega1284 - since most of them affect
> context switch code, I made a simple stress application that spawns 4
> tasks. First reads data from serial port, stores-and-forwards them
> through tasks 2 and 3 and finally to a task 4 which writes them back to
> serial port to be verified for integrity by the PC side. As long as no
> new data are arriving, currently held data is constantly shuffled
> through registers and stack. None of the tasks ever sleep, forcing as
> many context switches as possible. I ran this application for a few
> hours and no data corruption was observed.
>
> Please note that I do not have a Github account so, if these patches are
> accepted, I will be relying on someone else picking them up and doing a
> PR.
>
> I will attempt to post these patches through the mailing list, there is
> also a git repository nuttx.git available at git.kerogit.eu through
> HTTP/S . The branch with the changes is called avr_fixes_rfc1 and is
> based on upstream's releases/12.9 . It also rebases cleanly onto current
> master if that is the more preferred location where to put these.
>
> Description of the patches follows:
>
> === 0001 fixed function name ===
>
> Mistyped function name in Documentation resulting in something not
> present in the sources.
>
> === 0002 fixed typo in Kconfig ===
>
> Missing letter.
>
> === 0003 save RAMPZ register into thread context ===
>
> RAMPZ is an extension register used when accessing program memory
> (flash) from the program itself. It is concatenated with value in
> pointer register pair Z. Since user applications can reasonably be
> expected to load their data from program memory, this register should be
> saved into the thread context as well.
>
> That said, it's quite unlikely to hit the bug (reading from incorrect
> address) fixed by this patch - there are three conditions that need to
> be met:
>
> - forced context switch of a task that is currently reading from program
> memory
> - any other tasks that receives CPU time also needs to read from program
> memory
> - program memory address of those tasks must differ in RAMPZ value,
> which is the highest byte
>
> Meeting the last condition is somewhat unlikely because all data stored
> in program memory tends to be placed in a single region by the compiler.
> So to trigger the bug, this region would need to be large enough to
> cross 64kB boundary. Considering most supported devices have 128kB of
> flash in total and NuttX itself needs close to 64kB with very few
> features enabled, it is quite unlikely to have this much const data and
> NuttX and still an useful program.
>
> Still feels worth fixing though, impact on existing users should be
> negligible - few bytes of memory spent and few extra instructions. The
> fix also applies to ATmega2560 with 256kB of flash where it is more
> likely to have a program that can actually hit this.
>
> Note that similar bug still exists in ATmega2560 with its EIND register.
> This register is used to extend address register pair used for making
> indirect jumps (making jumps to addresses beyond 128kB boundary
> possible.) This is not addressed by this patch as I don't have such chip
> to be able to test the change.
>
> === 0004 flag microcontrollers that have RAMPZ register ===
>
> Enables code from previous patch on all currently supported chips.
>
> === 0005 replace AVR __flash qualifier with __memx for some chips ===
>
> This one may need some feedback. This patch changes __flash qualifier to
> __memx in IOBJ macro. These qualifiers instruct the compiler to place
> (or, rather, keep) const data in program memory and read it from there.
> The difference is that __flash is only able to address 64kB memory size
> while __memx extends the pointer size to 24 bits. The (possible) issue
> here is that this definition of IOBJ is currently restricted to
> compilation with AVR toolchain which I don't use and therefore can't
> test. However, I believe that their toolchain is based on gcc (which I
> use) and therefore behaves the same. At the minimum, the __memx
> qualifier is supported there for sure as it is the value used in IPTR
> macro already present in the code.
>
> Anyway, with this changes and with corresponding configuration options
> enabled, the code compiles and - judging from the assembly - seems to do
> the right thing (ie. does respect the __memx qualifier from IPTR macro
> used in parameters for syslog and syslog-related functions.)
>
> Using __flash qualifier as it is used now may cause bugs when const data
> is placed to memory regions above 64kB. As of now, compiler tends to put
> them at the beginning of the .text section so it is again unlikely to
> happen - as long as the data isn't large enough. And again, might be
> worth fixing since impact on existing users should be negligible. For
> starters, this is currently only used for debugging code and the
> associated cost of using 24 bit pointers is already paid in current form
> because of IPTR being set to __memx
>
> This change does not apply for devices with 64kB or smaller program
> memory (although there is no such device currently supported.)
>
> === 0006 fixed PC load on chips with more than 128kB program memory ===
>
> The original code uses Y pointer register pair to load 3 bytes from
> given memory address. However, using subi (SUBtract Immediate)
> instruction here is incorrect as it only affects the register it is
> called upon - YL, low byte from the Y register pair. High byte remains
> unchanged, resulting in incorrect load if the lower byte value wraps.
>
> The correct instruction for this would be sbiw (SuBtract Immediate from
> Word) which operates on the whole pair. The fix does not do that though
> - instead, the Y register pair is preloaded with value incremented by 1
> and loads are executed using pre-decrement in the load instructions.
> This both fixes the bug and also saves a bit of CPU time. The value in Y
> register then ends up being different compared to the original code but
> that should not matter since - as far as I can see - it is not used and
> is later overwritten.
>
>
> All the feedback and help is appreciated and my apologies for this text
> being this long
>
