[PATCHES] Fix PPP in libbsd and optimize ATSAM console

Mon Feb 7 16:04:47 UTC 2022

Hello Gedare,

Am 07.02.22 um 16:40 schrieb Gedare Bloom:
> On Wed, Feb 2, 2022 at 8:20 AM Christian MAUDERER
> <christian.mauderer at embedded-brains.de> wrote:
>>
>> Hello,
>>
>> I received no comments for these in the last two weeks. I assume there
>> are no further objections and I can push the patches to master in about
>> a week so that the bug is fixed at least for 6.
>>
> Yeah, that's fine with me, thanks for getting it tested on another
> BSP, and for following up.

Thanks for the response.

> 
> If you can advise how the API change affects other BSPs on 5, I think
> that can be accepted with a proper release note/documentation of what
> those users may expect.
There are two relevant changes in cpukit/include/rtems/termiostypes.h:

1. I added a field to the struct rtems_termios_tty. With that the binary 
interface changes.

2. I added a parameter to one of the line discipline functions.

-  int (*l_start)(struct rtems_termios_tty *tp);
+  int (*l_start)(struct rtems_termios_tty *tp,int len);

If someone is using the line discipline functions that should trigger a 
warning that the function prototype doesn't match. I think that an extra 
parameter in a function pointer is just ignored otherwise. At least I 
don't know of a case where it would have an effect. Please correct me if 
I'm wrong.

Best regards

Christian

> 
>> Best regards
>>
>> Christian
>>
>> Am 27.01.22 um 13:25 schrieb Christian MAUDERER:
>>> Hello,
>>>
>>> again: What can I do to make the patches acceptable?
>>>
>>> Best regards
>>>
>>> Christian
>>>
>>> Am 21.01.22 um 11:57 schrieb Christian MAUDERER:
>>>> Ping.
>>>>
>>>> Am 18.01.22 um 12:22 schrieb Christian MAUDERER:
>>>>> Hello,
>>>>>
>>>>> I noted that I still have this patch set open. I first posted it in
>>>>> August 2021 and later pinged it in September 2021. Both times no
>>>>> conclusion has been found. I would like to finally finish this topic
>>>>> and get the patches in an acceptable state. To simplify it a bit,
>>>>> let's only discuss the following two:
>>>>>
>>>>> * [PATCH rtems 2/2] termios: Pass number of sent chars to l_start
>>>>>     https://lists.rtems.org/pipermail/devel/2021-August/068892.html
>>>>>
>>>>> * [PATCH rtems-libbsd] ppp: Fix transmitting data
>>>>>     https://lists.rtems.org/pipermail/devel/2021-August/068893.html
>>>>>
>>>>> The third one (or rather first one) is a BSP specific optimization
>>>>> and not necessary for the other two. I'll not push that together with
>>>>> these two. I'll start a separate discussion for it as soon as we are
>>>>> done with these two.
>>>>>
>>>>>   From my point of view, the best case would be to apply the patches
>>>>> to master and 5. Back when I started the discussion I created tickets
>>>>> for both:
>>>>>
>>>>>     https://devel.rtems.org/ticket/4493
>>>>>     https://devel.rtems.org/ticket/4494
>>>>>
>>>>>
>>>>> @Gedare: In the following mail thread you asked about tests on
>>>>> further BSPs:
>>>>>
>>>>> https://lists.rtems.org/pipermail/devel/2021-August/068908.html
>>>>>
>>>>> I now finally tested the patches on an i.MXRT BSP. PPP works with the
>>>>> patches and doesn't work without them on that target. That's what I
>>>>> expected. The patches are tested on three serial drivers now:
>>>>>
>>>>> - ATSAM and i.MXRT: Don't work without patches. Work with them.
>>>>> - SC16IS752: Works for small packets only without patches. Works well
>>>>> with them.
>>>>>
>>>>>
>>>>> @Chris: We had two open discussions:
>>>>>
>>>>> 1. A style topic:
>>>>> https://lists.rtems.org/pipermail/devel/2021-August/068912.html
>>>>>
>>>>> Like said there, I would prefer not to reformat the whole file as
>>>>> long as we don't do that at least a bit automatic for most files.
>>>>>
>>>>> 2. A discussion about the direction and the necessary API change:
>>>>> https://lists.rtems.org/pipermail/devel/2021-September/069468.html
>>>>>
>>>>> Like I explained: That PPP bug exists basically on all BSPs.
>>>>> Depending on how many bytes a serial driver can process in it's write
>>>>> function, PPP works better or worse. My assumption is that at the
>>>>> moment, the PPP runs only on BSPs where it is "well enough" that no
>>>>> one noted the bug.
>>>>>
>>>>> To fix that bug, the PPP needs the information how many characters
>>>>> have been sent out. My patch set uses our RTEMS default path via
>>>>> rtems_termios_dequeue_characters().
>>>>>
>>>>> I'm not entirely happy that I had to change the API. But I didn't
>>>>> find a better solution and the API is still compatible. It will only
>>>>> trigger a warning if someone doesn't use that extra parameter. If you
>>>>> have a better idea how the bug could be fixed, I'm open to adapt the
>>>>> patches. But I need a suggestion what would be a better solution
>>>>> because - like I said - I didn't find one.
>>>>>
>>>>> Best regards
>>>>>
>>>>> Christian
>>>>>
>>>>> Am 12.08.21 um 13:41 schrieb Christian Mauderer:
>>>>>> Hello,
>>>>>>
>>>>>> this set of patches fixes PPP. Basically the current implementation in
>>>>>> libbsd can't work with console drivers that can't buffer a lot of
>>>>>> characters. The pppstart() function just assumes that the low level
>>>>>> console write can send an arbitrary number of characters without
>>>>>> checking how many characters are actually send.
>>>>>>
>>>>>> In the extreme case of the ATSAM interfaces (only one character can be
>>>>>> send at once) it's not even possible to establish a PPP connection with
>>>>>> that. For UARTS that have some FIFO establishing might work but bigger
>>>>>> packets won't go through. I opened tickets for 6 and 5 here:
>>>>>>
>>>>>> https://devel.rtems.org/ticket/4493
>>>>>> https://devel.rtems.org/ticket/4494
>>>>>>
>>>>>> I would like to apply the patches to both branches (5 and 6).
>>>>>>
>>>>>> The solution I implemented in this patch set is the following: PPP
>>>>>> output processing is done in the line discipline function l_start (or
>>>>>> rather the function where l_start points to: pppstart). Our device
>>>>>> writes don't return how many characters have been sent. Instead, that
>>>>>> feedback is given via rtems_termios_dequeue(). Luckily that's the
>>>>>> function that calls the l_start function.
>>>>>>
>>>>>> The RTEMS patch for termios extends the l_start function so that it
>>>>>> gets
>>>>>> the number of characters as a second argument. This extension shouldn't
>>>>>> be a problem for existing code. In the worst case it will trigger a
>>>>>> warning that the function doesn't match the pointer type. But in the
>>>>>> linked code, that additional argument will just be ignored.
>>>>>>
>>>>>> The libbsd patch extends the pppstart function so that it then handles
>>>>>> that new information.
>>>>>>
>>>>>> Patches belonging to this set:
>>>>>> * [PATCH rtems 1/2] bsps/atsam: Improve UART / USART tx performance
>>>>>> * [PATCH rtems 2/2] termios: Pass number of sent chars to l_start
>>>>>> * [PATCH rtems-libbsd] ppp: Fix transmitting data
>>>>>>
>>>>>> Best regards
>>>>>>
>>>>>> Christian
>>>>>>
>>>>>
>>>>
>>>
>>
>> --
>> --------------------------------------------
>> embedded brains GmbH
>> Herr Christian MAUDERER
>> Dornierstr. 4
>> 82178 Puchheim
>> Germany
>> email: christian.mauderer at embedded-brains.de
>> phone: +49-89-18 94 741 - 18
>> fax:   +49-89-18 94 741 - 08
>>
>> Registergericht: Amtsgericht München
>> Registernummer: HRB 157899
>> Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler
>> Unsere Datenschutzerklärung finden Sie hier:
>> https://embedded-brains.de/datenschutzerklaerung/

-- 
--------------------------------------------
embedded brains GmbH
Herr Christian MAUDERER
Dornierstr. 4
82178 Puchheim
Germany
email: christian.mauderer at embedded-brains.de
phone: +49-89-18 94 741 - 18
fax:   +49-89-18 94 741 - 08

Registergericht: Amtsgericht München
Registernummer: HRB 157899
Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler
Unsere Datenschutzerklärung finden Sie hier:
https://embedded-brains.de/datenschutzerklaerung/