Hi
As for your second paragraph - have a look into the avrdx directory.
The first depends on if you are strictly speaking about ATmega and
AT90USB or if you mean all 8-bit MCUs in the AVR architecture. If you
only mean those two, then I can't really provide much input, don't have
experience with the AT90USB family. From a quick glance to the
datasheets it looks that both the interface and peripheral are the same
so it may be possible to merge the implementations relatively easily...?
If you want to unify everything, ie. the DA/DB family too, that's a
different story. The peripheral in the new family isn't the same - has
more features. The interface also differs slightly (different registers,
bit positions are not the same either.)
It's probably still possible to unify this code with ATmega and AT90USB
with a lot of #ifdefs... not sure if that is worth the work though.
One thing I would be slightly worried about is that if the driver moves
to common code, it may at some point impede development of features that
the older chips do not have (RS-485, IrDA, noisy line detection etc.) I
would guess high risk of spaghetti code there.
Best regards
On 2025-06-03 16:03, Matteo Golin wrote:
Hello KR,
Mildly related (about serial on AVR): I've noticed that the serial
implementation between avr chips (atmega, at90usb, etc) have almost
identical implementations besides the #define names of the IRQ numbers
and
how many UART interfaces they implement. I'm under the impression that
all
AVR chips have the same interface for controlling the UART peripheral.
What
do you think about unifying the serial implementation into `common`?
There's already `CONFIG_AVR_USARTn` for choosing which USARTs are
available, so the only other change would be making sure to get the IRQ
numbers correct.
I also wanted to look into whether or not the avr/io.h header makes the
interfaces available as either a struct or with just the addresses (not
the
pointer magic register defines). I noticed that the implementations of
usart1 and usart0 functions are also almost identical except for the
register naming, and thought it would be possible to instead put the
registers in the `priv` of the `uart_dev_s` so the same functions can
be
used for any UART interface. Might save some space in memory, too.
Best,
Matteo
On Mon, Jun 2, 2025 at 3:21 PM MIGUEL ALEXANDRE WISINTAINER <
tcpipc...@hotmail.com> wrote:
wow,
I see that sorry!
________________________________
De: Alan C. Assis <acas...@gmail.com>
Enviado: segunda-feira, 2 de junho de 2025 18:58
Para: dev@nuttx.apache.org <dev@nuttx.apache.org>
Assunto: Re: Sv: RFC: fix race conditions in drivers/serial/serial.c
Hi Miguel,
Which kind of modification are you looking for?
RS-485 is already supported:
https://embeddedrelated.com/showarticle/1715.php
BR,
Alan
On Mon, Jun 2, 2025 at 3:52 PM MIGUEL ALEXANDRE WISINTAINER <
tcpipc...@hotmail.com> wrote:
> should be good change too this driver to enable the RS-485
> ________________________________
> De: kr....@kerogit.eu <kr....@kerogit.eu>
> Enviado: segunda-feira, 2 de junho de 2025 16:58
> Para: dev@nuttx.apache.org <dev@nuttx.apache.org>
> Assunto: Re: Sv: RFC: fix race conditions in drivers/serial/serial.c
>
> Hello,
>
> I noticed a review from Allan C. Assis about a spelling error. Updated
> patches are attached and also available in repository in uart_fixes_rfc2
> branch. (Sorry about the extra work, I was even trying to check the word
> using web search... but I didn't spot that the search autocorrected it
> so my conclusion was that the spellchecker I also used is just tripping
> on the plural.)
>
> Best regards
>
> On 2025-06-02 09:39, alin.jerpe...@sony.com wrote:
> > Hi KR,
> >
> > thank for submitting the patches
> >
> > They have been uploaded to mainline and are under review
> > https://github.com/apache/nuttx/pull/16466
> >
> >
> > Best regards
> > Alin
> > ________________________________
> > Från: kr....@kerogit.eu <kr....@kerogit.eu>
> > Skickat: den 1 juni 2025 22:42
> > Till: dev@nuttx.apache.org <dev@nuttx.apache.org>
> > Ämne: RFC: fix race conditions in drivers/serial/serial.c
> >
> > While going through the code in drivers/serial/serial. c, I noticed
> > this comment: The head and tail pointers are 16-bit values. The only
> > time that the following could be unsafe is if the CPU made two
> > non-atomic 8-bit accesses to obtain the 16-bit
> >
> >
> > While going through the code in drivers/serial/serial.c, I noticed this
> > comment:
> >
> > The head and tail pointers are 16-bit values. The only time that
> > the following could be unsafe is if the CPU made two non-atomic
> > 8-bit accesses to obtain the 16-bit head index.
> >
> > This is what happens for (at least) AVR architecture. These are 8bit
> > microcontrollers and as such, they will fetch the 16-bit value in two
> > 8-bit load instructions; interrupt routine may execute between those
> > and
> > change the value being read. This will result in corrupted read (one
> > byte of the value will be from pre-interrupt state and the other from
> > post-interrupt state.)
> >
> > This patch introduces CONFIG_ARCH_LD_16BIT_NOT_ATOMIC configuration
> > option, which is automatically selected for architectures known to be
> > unable to load 16bit values in one instruction. (It is currently only
> > set for AVR. I presume it might be also useful for Z80 but I do not
> > have
> > any experience with that architecture so I did no changes there.) When
> > this configuration option is set, reads of recv.head and xmit.tail are
> > enclosed with enter_critical_section and leave_critical_section calls
> > to
> > prevent interrupt handlers from running, if needed. Not all reads need
> > to be protected this way - some are already in existing critical
> > sections and some happen with the UART-specific interrupt disabled.
> >
> > If the configuration option is not set, the code simply loads the value
> > into a local variable. Subsequent direct uses of the unprotected
> > volatile variable are replaced with the local variables for both cases.
> >
> > There is also a related change that only applies when
> > CONFIG_SERIAL_IFLOWCONTROL_WATERMARKS is set. Aside from the protection
> > selected by CONFIG_ARCH_LD_16BIT_NOT_ATOMIC, this patch also fixes
> > calculation of the nbuffered value. This calculation is not running
> > with
> > interrupts disabled and value of rxbuf->head can change between the
> > condition and actual computation. Even if the load itself is atomic,
> > this leads to TOCTOU error.
> >
> > Impact: this change should not impact architectures that do not benefit
> > from this change at all unless CONFIG_SERIAL_IFLOWCONTROL is set. If
> > CONFIG_SERIAL_IFLOWCONTROL is set, the only change that remains in
> > effect is the fix of the TOCTOU error.
> >
> > Testing: Patch was tested with rv-virt:nsh - disassembly of functions
> > from drivers/serial/serial.c was compared and did not change after the
> > patch was applied - with one exception. The exception was an address of
> > a global variable, I assume it was caused by change of g_version length
> > (which notes that the tree is dirty) and is therefore inconsequential.
> > CONFIG_SERIAL_IFLOWCONTROL was not set in this test.
> >
> > Configuration with CONFIG_SERIAL_IFLOWCONTROL set was tested by
> > building
> > it for AVR DA/DB. Configuration with CONFIG_SERIAL_IFLOWCONTROL unset
> > was tested by custom echo application running on AVR128DA28 for a few
> > hours.
> >
> > I also ran CI test: 1044 passed, 10 skipped, 14 deselected, 3 warnings
> > in 2463.23s (0:41:03)
> >
> >
> > On a related note, it seems to me that there may be a bug in echo
> > handling in uart_readv. If dev->tc_lflag & ECHO is true, then
> > uart_putxmitchar is called. This function adds the received character
> > to
> > transmit buffer and increments xmit.head value. However,
> > uart_putxmitchar is also called from uart_writev; this function states
> > "Only one user can access dev->xmit.head at a time" and takes the
> > dev->xmit.lock. As far as I can see, uart_readv does not take the lock
> > and may interfere with uart_writev. Evaluating and solving this
> > properly
> > is currently above my skillset though. (If this is even considered
> > serious enough to warrant a fix.)
> >
> >
> > The patch series also fixes a typo in drivers/serial/serial.c
> >
> >
> > Both patches are attached to this message and also available in a git
> > repository nuttx.git at git.kerogit.eu accessible through HTTP/S.
> > (Trying to prevent bot traffic by not posting the URL in
> > machine-readable form.) The relevant branch is called uart_fixes_rfc1.
> > If the patches are acceptable, I would like to ask someone with GitHub
> > account to open a PR (I don't have a GH account.) Any comments or
> > suggestions are welcome.
>
