Addition of buffering for serial, Change to default baud rate about t-962-improvements HOT 18 CLOSED

Hmm, interesting - I had hoped that all those really dumb PL2303 converters would have self destructed by now. Man that is a really lousy design (I've had numerous such chips fail in the weirdest of ways). :)

Anyway, 115200 is definitely on the slow side but I can live with that together with some buffering. The interrupt-safeness of the code is related to the 1-wire stuff that will need to kill the interrupts while the time critical bitbanging happens. When someone has time and energy to figure out how interrupts work in this chip that can always be added.

Also regarding the Uart_Work function, if you want something to run every run of the Sched_Do loop the normal way I've done it is to register a work function and always return 0 from it, that will force a new call during the next iteration. That still makes it possible to selectively disable it if one would feel so inclined.

A downside with this buffering code is that no UART output will show up until the Sched loop starts to run (that means that if initialization code hangs somewhere between Serial_Init and Sched loop it will be difficult to see what actually happened. This scenario would be solved by adding the Uart_Work code as two interrupt handlers instead.

There seems to be a warning introduced by this code though if you look at the Travis-CI "Details" link on the pull request (https://travis-ci.org/UnifiedEngineering/T-962-improvements/builds/44531757):

src/serial.c:63:1: warning: control reaches end of non-void function [-Wreturn-type]

I don't have the possibility to give this a test spin tonight but will look into it tomorrow evening. However the more I think about this the more I think we're going to need interrupts in order to not create UART regressions with this buffering scheme. I can go over the rest of the code to make sure that we're good to go with interrupts. Makes sense?

from t-962-improvements.

Hmm, interesting - I had hoped that all those really dumb PL2303 converters would have self destructed by now.
Mine's brand new ;-) Actually the newer chips do support faster baud-rates so we can crank it back up, but they don't support arbitrary baud-rates. So they support 1.something MB/s for example, but not 1MB/s or 2MB/s. I didn't want to do the math of the divider for the faster baud rates so stuck with something easy to get from the XTAL.

The interrupt-safeness of the code is related to the 1-wire stuff that will need to kill the interrupts while the time critical bitbanging happens.

I didn't even catch that - I was looking at all the feed statements (PLL feed, WDT feed). According to the data-sheet if an interrupt occurs inbetween them it gives you a nice data abort! Although we could wrap large sections of the code in interrupt-disabled sections, since the UART interrupts aren't that critical I think.

However the more I think about this the more I think we're going to need interrupts in order to not create UART regressions

Indeed - actually I didn't think about it either but the startup messages will be chopped off too, since it writes > 128 chars before the work function is called. In this case I must not have noticed since the normal printf()'s looked good.

Enabling interrupts would be much nicer - this way can just use the UDRE/TFRE interrupts (or whatever they are called on this device) to detect when we have a char/can send a char.

I can go over the rest of the code to make sure that we're good to go with interrupts.

Yup - I assume everyone will get busy over holiday time so no worries. Once they look good I can refactor my code to use interrupts.

from t-962-improvements.

Good catch! You're right, need to make sure interrupts are disabled around the re-arm of the watchdog too. I'll keep that in mind!

from t-962-improvements.

I have added IRQ support than seems to be working on the irq-integration branch (tried it with the RTC CIIR tick). Will merge it into master sometime in the next few days when I see that it did not cause any side effects. It already deals with the disabling of interrupts around the onewire stuff and watchdog re-arm as well.

from t-962-improvements.

What's the status on this? I'm having problems with the 2Mbps baud too, 115200 works fine. Can we merge that change?

from t-962-improvements.

Christmas happened I think ;-) I need to refactor this code to use the irq-integration branch instead, but I was waiting for that branch to be merged back to master before I did those changes.

from t-962-improvements.

Yeah, Christmas happened.. :) Ah, let's see what we can do about merging the irq stuff into master. I was thinking that we would be able to use the irq branch to test this out properly with the pull-requests going there first and then I merge a complete solution into master where all the buffering is done. That way we can have multiple shots at getting it right and do it step by step and not break master in the process.

What do you think, could you work with the irq-integration branch and get things up and running and then direct the pull requests there? Work-in-progress code that isn't working as it should yet is perfectly fine on that branch, then we'll see early how things are progressing.

from t-962-improvements.

Sure thing - will do! I had seen irq-integration was behind master so was trying to avoid any merge conflicts in case you were going to be bringing it up to master. I might bring it up to speed (i.e. merge all the master commits into it) on my fork and work from there then.

from t-962-improvements.

Awesome! The changes currently on master should have no impact on the irq/buffering stuff, but feel free to test with or without them!

from t-962-improvements.

I've committed this to my branch (see https://github.com/colinoflynn/T-962-improvements/tree/irq-integration). I changed the default baud to 230k - there was an occasional WDT reset with 115k baud. But I haven't done much other testing yet so if you get a chance try running it in your oven.

from t-962-improvements.

I have now run a few successful reflow sequences at 115k2 with this code without any watchdog bites, but I did notice that there are ways to make this code fall over, triggering the watchdog bite. Sending large amount of RX data to it triggers an almost immediate bite.

Just out of curiosity I set the baudrate really low (9k6, U0DLM=1, U0DLL=194) and increased the watchdog timeout to 4 seconds (9k6 is more than enough to sustain the normal UART flow). I can see that the code actually gets stuck (and just sits there for 4 seconds until watchdog bites) 5 to 20 seconds after boot. Initial startup will look garbled at that low speed, that can be ignored. There's got to be a timing issue with re-enabling or disabling the sub-interrupts going on here..

Also I think it's better to actually check the correct bit in uart_putc:
add_to_circ_buf(&txbuf, thebyte, !!(VICIntEnable & (1<<VIC_UART0)));
(and update the comment about always calling it in blocking mode).

I don't have a solution to the wdog bite issue yet, but increasing baud rate to 230k will not solve the actual issue. Do you have any ideas as to why it would get stuck? Try running it at 9k6 and see if you get similar results. I'll think about it some more and see if there's anything obvious..

from t-962-improvements.

Alright good to know a method of forcing the WDT reset! Will take a look in a bit - I suspect the main issue might be that it needs portions of the circular buffering code to be atomic, as I had copied that code from the polling branch which didn't care about interrupts. I had just remembered that...

from t-962-improvements.

Just as an update - I've got a new branch with some debug tests in it, but remain unsure where the underlying issue is. I made the circular buffer stuff interrupt-safe (for now which means it never blocks, and always drops characters if no room), and also added a spurious interrupt handler for the RX interrupt. I don't think I'll have a chance to look at this more for a few days at least, so will leave this as-is for now.

Note that if I disable the onewire code I don't have problems either (i.e. force onewire discovered devices to be 0). It may be some issue with enabling/disabling interrupts somewhere there too. I don't think this is being caused by the spurious interrupt, as it sounds like it's going into a loop, whereas I would expect a spurious interrupt to produce an immediate reset type error (at least without the handler programmed).

from t-962-improvements.

Hmm, interesting! I'll dive in and see if I can find out what might be wrong with enabling/disabling interrupts. As the issue seems to disappear when onewire isn't constantly fiddling with it that sure sounds reasonable! I spent some time adding simple exception handling to the startup code to make sure that we didn't get stuck because of some data abort or something - turns out that we did not.

from t-962-improvements.

By the way, no irq disable/restore calls are needed around the initial wd feed instructions in main as the vic isn't enabled yet, just noticed that commit. I'm continuing my investigation, this is probably (as always) some mundane detail gone wrong.. :)

from t-962-improvements.

I think I have it figured out.. I changed the way interrupts were disabled (turn them off at the core level instead of using the VIC) which helped the situation, the I minimized the amount of time spent with IRQs disabled during OneWire operations. You can now run with a 256 byte buffer at 9600 bps (using divisor 360) and it will chug along just fine. I pulled in the stuff from irq-integration fork into my irq-integration branch together with some fixes.

Now I think we're close to merging it onto master - I'll let the code run for a while in my oven here and see if it acts up first. Anyone else are more than welcome to try the code from the irq-integration branch out as well and report any oddities. I updated the rest of the functionality from master as well so right now irq-integration is ahead.

I could not make the code misbehave even when throwing garbage into the RX path either (should be ignored, which it seemed to be)

from t-962-improvements.

Great news! I don't know how SRAM usage is, but you can turn down the buffer size if required too. Thanks for finding that issue out, nice to have it all merged back in.

from t-962-improvements.

I had the buffer size all the way up to 4096 bytes (times 2) at one point with plenty of spare RAM still. :) The rest of bss plus initialized data is less than 2kB out of the available 16kB. I think 256 bytes is a good number for now, it's the smallest rounded-up power-of-two size that can deal with two full lines of logging output without stalling (during reflow mode switching for example).

from t-962-improvements.