Hi KR, On Mon, Apr 21, 2025 at 6:22 AM <kr....@kerogit.eu> wrote:
> Hello, > > I would like to submit some patches for review and comments. The overall > goal is to provide support for AVR DA/DB family of chips but some of the > patches are more generic - fixes in other AVR chips and some typofixes > elsewhere. > > I will attempt to post these patches through the mailing list again, > they are also available in a git repository nuttx.git at git.kerogit.eu > through HTTP/S . (Sorry for the "cryptic" URL, trying to avoid at least > some bot traffic.) First five attached patches are those posted > previously (see > https://lists.apache.org/thread/2yd84dvpw6cznz1do16b0glq77z3l4bh ) , > those were altered based on Alan C. Assis's review and also patches > originally numbered 3 and 4 were squashed into one. In the repository, > they are in a branch named avr_fixes_rfc2. > > Thank you, I just submitted the first set of patches that you fixed: https://github.com/apache/nuttx/pull/16268 > The remaining patches are packed into a single tar.gz archive and also > available in a branch named avrdx_rfc1. All of this work is based on > upstream's releases/12.9 . It should also rebase cleanly (it did when I > last tried) onto current master if that is the more preferred location > where to put these. Please note that I do not have a GitHub account so > if people are interested in having these merged, the pull request will > need to come from someone else. > > Ok. If possible, could you explain the issue with github? > Before going into more specific description for each patch I have a > question about errors from the checkpatch tool. I corrected most things > that it pointed out but I have a problem with a check for Mixed case > identifiers. As with the other AVR chips, support for these newer ones > includes from avr/io.h file. Unlike previous chips though, the naming > scheme of macros defined in the header file fails this test. As an > example - macro for bit position in I/O register that controls USART > interrupt enable is defined as "#define RXCIE1 7" for the older chips. > For the newer ones, two macros are provided: "#define USART_RXCIE_bm > 0x80" and "#define USART_RXCIE_bp 7". (There are also additional macros > with other suffixes related to bits that have meaning in groups; those - > to my knowledge - do not have direct equivalent in io.h for the older > chips.) > > My question is what to do with these. The options I came up with are: > > - grant an exception for suffixes gc, bm, bp and gp - there already seem > to be some exceptions in the nxstyle tool for some suffixes > - make a copy of the header file(s) and change names of the macros > - use magic numbers instead > > Obviously, options 2 and 3 feel all kinds of wrong but I am not sure if > option 1 is permissible for new code. > > I think in this case we could add _bm and _bp as exceptions as well. > Other than that, there are few other things to note: > > 1. Contributing page on NuttX website sugguests that the patches are to > be squashed. I did that for some part, keeping logical steps in the > development separated. This way if eg. the support for serial driver is > found to be in need of changes, other code can still be merged. If the > patches should be squashed even more, let me know. > > Our Documentation is not perfect and has many issues that we need to fix (i.e. suggesting merge instead of rebase, etc). The squash only applies when you break down many small modifications to the same logical thing. It is important to have separated commit when they apply to different logic, different subsystems, etc > 2. These patches depend on "nuttx/clock: make NSEC_PER_USEC and others > long" (ac42add946) being applied. It is only present in NuttX master > branch and needs to be cherry-picked for testing on the branch based on > release 12.9 > > Could you please give more details here? > 3. The patches are ordered in such a way that applying any number of > them in order should yield code that works. In other words - if some > patch is deemed problematic, all patches before it can still be > considered suitable for merging and the result should work (albeit with > some functionality removed.) > > The checkpatch here didn't find any issue, everything seems fine! > The description of the patches follows: > > [ Fix alignment of .text section if .progmem is used ] > > This patch fixes an intermittent error I found during development where > the linking step failed when using program memory constants. It turned > out that the error depends on the length of the constants - if the total > length of the data was odd, it caused misalignment in following sections > which needs to be word-aligned. This patch should fix that. Note that I > was only able to test this change for the ATmega chips and only by > compilation. It does work with the DA chip though. > > Fine and you included this info in the commit log message! Very nice! > [ Decouple enabling of IOBJ/IPTR qualifiers from debugging ] > > Currently, it is only possible to mark const variables with the IOBJ > (ie. do not copy to RAM) qualifier when arch-specific debugging is > enabled. In my opinion, doing so is worthwhile outside of debugging > scope - for example for syslog/printf format strings. It should be noted > though that most of the core NuttX code is not marked with corresponding > IPTR qualifier so usage of IOBJ is currently quite limited (pretty much > to just syslog/printf and not even all syslog functionality is > available.) > > This is a change to already existing code but it should have minimal > effect on existing users as it is kept as an opt-in feature. > > Ok > [ Enable eliminating unused sections with GCC ] > > More precisely, this patch allows marking the GCC toolchain as a GCC > toolchain (ie. GCC toolchain option in AVR-specific Kconfig will select > ARCH_TOOLCHAIN_GCC.) The direct consequence is that > DEBUG_OPT_UNUSED_SECTIONS is enabled (it is marked default Y) and unused > code is removed from the resulting binary. Other architectures have this > enabled, program code savings can be quite relavant for these > flash-constrained chips and as far as I understand it, the downsides are > negligible. > > Ok, not a big issue, and we can improve later if needed! > However, linker scripts need to account for this and, for existing > chips, they do not. Most notably, .vectors sections need to be flagged > as KEEP, otherwise the linker removes them too. Since I can only test > the DA chips properly, I enabled this indirectly. Instead of selecting > ARCH_TOOLCHAIN_GCC unconditionally, the GCC toolchain only enables > additional option marked as default N. This additional option then > selects ARCH_TOOLCHAIN_GCC, if set by user; its description provides > warning about the linker script. > > I think we will need to test on some Atmega board, like the Arduino Mega board. > Linker script for the DA chip board (later patch) marks the .vectors > section appropriately. I think that ATmega chips only need to mark the > .vectors section as well. As for the other chips - the AVR32 especially > - I have no clue. If someone else knows/can test the other chips and > adjust their linker scripts appropriately, this indirection can > eventually be removed. > > I'm CC Mr. Raman, he has more experience with AVR32, about he knows about it for normal AVR too. > [ Make the linker obey DEBUG_LINK_MAP ] > > Simple patch that adds a parameter to linker so it obeys this setting. > > Ok > [ Initial support for AVRnDx chips ] > > This patch adds support for the DA/DB family - with only single chip > being supported at this stage. The work is derived from ATmega family > with most features stripped off. > > At some point during the development, I encountered an error that > required me to analyze how the context switch works. Since that > knowledge may be useful for someone else too at some point, I created > context-switch-notes.rst document. I am pointing it out since I am not > sure if it is permitted to just put arbitrary documents in the > Documentation directory. (As a note - I am fairly sure the code works as > I described it but since I am not its original author, there may be > errors.) > > Ok > [ Boards - added AVR DA chip on breadboard ] > > This patch adds board counterpart to the new architecture code. I use a > breadboard for development since I don't own any commercially available > development board - hence the name. Hope using such board is > permissible. > > Linker script provided with the board is a copy of corresponding script > for ATmega chips with adjusted values. Also, .vectors table is marked to > be kept by KEEP (ie. not removed as unreferenced code.) Please note that > I have little knowledge when it comes to linker scripts so most of the > work here is copy & paste combined with guesswork. > > Cool! I bought this low cost AVRDB64 board: https://www.electrodragon.com/product/avr128db64-mini-develpment-board-avr128/ I will test it and submit support to the mainline. [ Provide tickless OS option ] > > This patch adds tickless alarm configuration option and related code > needed for this mode of operation of the system clock. > > Cool! > [ Added support for serial driver ] > > This patch adds support for generic USART driver in interrupt mode. > > Ok! > [ Added I/O interrupt multiplexer ] > > This patch accommodates drivers that use interrupts on pin change. There > is only a single interrupt vector for whole 8 bit port in AVR DA/DB > chips, which may prove problematic if multiple drivers want to claim > only some pins (not overlapping) of the same port. I did find out that > it is possible to enable support for attaching multiple handlers to a > vector but it felt like not running driver code needlessly might be a > better option. Intended user of this is board code which will attach > itself into the multiplexer and forward handler received from the > driver. In other words - no changes outside of AVR-related code are > needed. > > If the board does not use one port for multiple devices (driven by > different drivers) or the board code author chooses to go with the > multiple handlers per interrupt route, this can be simply not enabled in > the Kconfig. > > Understood! > [ Support for buttons handled by input driver ] > > This patch adds buttons to the breadxavr bread board. Although it does > not need to, it is using the GPIO multiplexer from previous patch. (As a > proof of concept and usage example.) > > That is good to prove that your implementation is working! > [ Added support for AVR128DA64 and AVR128DB64 chips ] > > There are multiple variants of the DA/DB family of chips but those > generally differ only in pinout and in how many instances of various > peripherals the chip has. This patch adds support for some other chips > besides the initial one. The code does compile but no other testing was > done for these. > > > Overall, the code seems to running well - did some overnight stress > testing. Also tried to run the test suite according to > https://nuttx.apache.org/docs/latest/guides/citests.html and got 1044 > passed, 10 skipped, 14 deselected, 3 warnings (I didn't expect failures > here because my changes don't touch things covered by the tests - I > assume.) > > If there is something unclear or something should be done differently, > please let me know. > KR, I have no words to say about this great work you did. Although your initial plan was to make NuttX on AVRDB useful for hobbyists, I'm sure it could be useful for companies as well. BR, Alan
