On 27/9/21 6:24 pm, Christian MAUDERER wrote: > same like for another patch set: There is a pending discussion for this one.
Yeap and so we need to resolve it. I have reordered your original post so the chat follows. I hope that is OK? > Is there still an open point left of can I push the patches? What happens if another BSP needs the same change? Is it also OK for the same reasons or is this the only one? As the PPPD code needs specific drivers can the build in LibBSD be restricted to the BSPs that support it and no others? > Chris: Back when I posted the patch set you mentioned that you are not > entirely > happy about it in a discord chat (about 17.08.2021 6:50 in UTC time zone). The link follows but you need a login is: https://discord.com/channels/820452222382112799/820452222848335924/877082405545062410 Given discord does not seem to have a public archive we can access without an account I will paste the conversation here: kiwichris — 08/17/2021 @c-mauderer hi c-mauderer — 08/17/2021 Hello @kiwichris kiwichris — 08/17/2021 Just looking over the PPP patches c-mauderer — 08/17/2021 Great, thanks. [4:53 PM] Any problems with them? kiwichris — 08/17/2021 Can you please explain this fragment ... - if (ctx->transmitting && (csr & US_CSR_TXEMPTY) != 0) { + while (ctx->transmitting && (csr & US_CSR_TXRDY) != 0) { rtems_termios_dequeue_characters(tty, 1); + csr = regs->US_CSR; c-mauderer — 08/17/2021 That's in the ATSAM console driver. Basically the same construct is used for receiving. [4:55 PM] If the send buffer is empty, the first byte will more or less fall through and the TXRDY flag is still set. [4:55 PM] A second byte can be queued. kiwichris — 08/17/2021 I am concerned about the loop because it is dependent on the baurdate c-mauderer — 08/17/2021 you mean that on very fast baud rates it could theoretically loop forever? kiwichris — 08/17/2021 Slow? c-mauderer — 08/17/2021 It will always only loop one or two times. [4:58 PM] If nothing has been sent before, the first will fill the send buffer and the next will prepare one byte that is written to the register. kiwichris — 08/17/2021 Is it timed on the bit shift time out? c-mauderer — 08/17/2021 no kiwichris — 08/17/2021 But the dequeue is removing it from the queue which means it is sent. c-mauderer — 08/17/2021 it is given to the hardware and also it isn't yet on the line, it will be sent in any case. [4:59 PM] It's basically a 1 byte FIFO kiwichris — 08/17/2021 But you could loop more than once and if just one char why the loop on the dequeue? c-mauderer — 08/17/2021 You are right: The loop would have also be possible and slightly more efficient in the write. But it would have added quite a bit of logic to the write. kiwichris — 08/17/2021 c-mauderer — 08/17/2021 The loop here was the version that was simpler to read and more similar to the read path. [5:03 PM] But yes: It's a bit of a lazy solution. It's not the optimal one but it works well enough in that driver. kiwichris — 08/17/2021 I have a patch for the zynq that adds proper FIFO tx support. [5:04 PM] It will break. I have seen enough UART drivers to know the bit will stay set. [5:04 PM] And I have changed the transmitting flag logic because it look suspect to me @kiwichris It will break. I have seen enough UART drivers to know the bit will stay set. c-mauderer — 08/17/2021 You mean the TXRDY flag? kiwichris — 08/17/2021 I will chat to Joel and Gedare about the change for the extra args to the call. I am not sure about this part because it fixes only one driver and leaves a "functional bomb" in the code for another user to trip over. @kiwichris I will chat to Joel and Gedare about the change for the extra args to the call. I am not sure about this part because it fixes only one driver and leaves a "functional bomb" in the code for another user to trip over. c-mauderer — 08/17/2021 OK. But what are the alternatives? [5:06 PM] It provides the feedback to the pppstart function how many characters have been written. kiwichris — 08/17/2021 Yes. A specific baudrate and data flow and you hit a threshold and a character disappears from the stream and it would be hard to find. c-mauderer — 08/17/2021 Our termios doesn't really have another feedback than that. kiwichris — 08/17/2021 Yes. Do all other drivers still work with pppd? [5:07 PM] I know it does not. It is very bespoke to a console type termios c-mauderer — 08/17/2021 I haven't tested all. At the moment I have tested the ATSAM and the SC16ISxxx kiwichris — 08/17/2021 If the pppd depends on it and the driver ignores it how can it? [5:08 PM] I must be missing something here c-mauderer — 08/17/2021 I don't think that the pppd ever really worked before that patch. That is a bug that was there for a long time. It only wasn't really clear for all chips that had enough FIFO or that used a DMA. [5:10 PM] If you take a look at the call in pppstart: (*tp->handler.write)(ctx, (char *)sendBegin, (ioffset > 0) ? ioffset : 1); : It just assumes that it can send any number of characters via the device write. [5:10 PM] That's not true for any driver. [5:10 PM] But some will put most of the packet (or all of it) in a FIFO. kiwichris — 08/17/2021 Sure this is a valid point. The other side is leaving a further function bomb where we do not know or document it. Could pppd use termios? (edited) [5:11 PM] Why is it directly access the drivers? c-mauderer — 08/17/2021 That has a history. The pppd has been added to RTEMS at least 20 years back. The one in libbsd is just a port from the original RTEMS one. [5:11 PM] Someone deeply integrated it into termios back than. [5:12 PM] It would be a really huge task to remove that integration. [5:12 PM] The most likely better alternative would be to just drop pppd some-when and use the user-ppp that FreeBSD uses now. [5:13 PM] But that would break the API. kiwichris — 08/17/2021 I have not looked at the code and so cannot comment. I am mixed minds about a change to drivers. If the pppd has not worked then it there value in the API? (edited) c-mauderer — 08/17/2021 It worked most likely well enough on some targets (due to FIFO or DMA) that it is used on some platforms. [5:15 PM] I was really annoyed that the pppd has a such a deep integration in our termios. The fix is basically only fixing the code that is there. [5:16 PM] I don't really like the code kiwichris — 08/17/2021 I understand there is no clear and simple solution here. We have a pppd send API and users of it and we have a single termios driver interface but this changes that. c-mauderer — 08/17/2021 I don't think that the termios line discipline functions are used often. I found the pppd and a mouse driver in RTEMS. Beneath that: The change might trigger a warning. But old code will still work. [5:17 PM] The additional parameter to the send function will just be ignored. [5:18 PM] Or do you know a platform where you can't pass a function that only uses one parameter to a function pointer that is called with two? kiwichris — 08/17/2021 There are other solutions equally not nice ... for example we just flag the pppd to be removed in the next version and the users need to step up? It has not been the RTEMS way and not nice but I would like to avoid fragmenting things in specific ways because the project ends up with the liability to clean it up. c-mauderer — 08/17/2021 I only touched that code because there is a user. Otherwise I wouldn't have touched it. kiwichris — 08/17/2021 It is not about the technicality of being able to do this, it is a question of do we want to do it? [5:19 PM] And what are the latent costs? c-mauderer — 08/17/2021 That someone who wants to rewrite our termios has to do just what you suggested first: Flag pppd out of date. kiwichris — 08/17/2021 I appreciate that and users are important. We can live with transitional changes c-mauderer — 08/17/2021 I don't think that PPP is used a lot nowadays. I know of two of our projects where a user uses it. kiwichris — 08/17/2021 I am mindful is the change going and everyone thinking all is OK [5:24 PM] I have used it in the past. I used it to connect metro-optical nodes where IP was sent over PPP over GRE in ISO end points in SDH networks. c-mauderer — 08/17/2021 Yes, I'm sure there are some users. Like I said: I know two myself. But I really hope that it will get less often used. Chris _______________________________________________ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel
