Actioned Issue 89 as feature/serial2

[gruvin]

This implements a second USART serial port on pins D1(TX) and (D0)RX.

Please see Issue 89 comments, in additional to prepended notes in README.md for further details.

[bkobkobko]

Hi @gruvin

Looks great! One very tiny point: I just wanted to make sure you knew there is a makefile target program-dfu that does what your "build" script does. This is nice so that if you don't need a clean compile, you can do make program-dfu and build and download.

[gruvin]

Build script? Oh crap! -- flash. That file was never supposed to have been staged. I'll remove it. Otherwise, no. I didn't know that! So thanks. Gosh.

I just attempted to analyse the cost in Flash and RAM for my changes. Have added those notes to README.md. It's more than I expected, at over quarter Kbyte for just Serial1, Serial2 not instantiated. I'm kinda disappointed. Perhaps the arguably ugly, 'if/then/else everywhere' method might have been better, after all. Oh well.

[gruvin]

EDIT: This comment can be ignored. The test mentioned in next comment invalidates this one, entirely.

Just realised that I'm dereferencing an additional level of pointers to rx/tx_buffer, using that _UMAP macro, in many places. This may account for quite a lot of extra machine instructions. Perhaps I should return to my earlier version, of storing a pointer to the appropriate USART_MAP array, like it was at d31730c.

Example:

I was using myUSART->usart_peripheral, instead of _MAP.usart_peripheral, which expands to USART_MAP[usartIndex].usart_peripheral, where myUSART was a local class pointer variable, initialised in the constructor. Presently, I am storing int usartIndex, instead.

I think in the second (and present code) case, there's extra instructions required to dereference USART_MAP[...]. Since this appears in quite a number of locations throughout the code, I believe it may be unnecessarily consuming signifiant code space.

Comments anyone?

If I get time tomorrow, I'll do a bunch of search/replaces in vim to test this theory. If it turns out I am right, then I suppose a regression to the pointer version would also get rid of the extra code readability issue, with the _UMAP macro, which couldn't hurt -- the irony being that the _UMAP macro etc was intended to save code space, actually. But I think I got it backwards. :-P

[gruvin]

I tested the above just now IE. reverted to use of pointers instead of USART_MAP[usartNum] array dereferencing. To my surprise, the final Flash size came out identical, to the byte. So I suppose the compiler's optimization is taking care of coding styles for me.

So, I think this is as optimal as I can get it. Pull request stands as-is, then. Thanks.

[towynlin]

Great, thanks @gruvin. I look forward to reviewing this. With a little luck I'll get to it today or tomorrow.

[towynlin]

I'm reviewing this now @gruvin — if my tests continue to go well I would like to merge it in before I tag v0.2.2 (like, in the next couple hours) — but I just noticed you haven't signed the CLA. It's here: https://docs.google.com/a/spark.io/forms/d/1_2P-vRKGUFg5bmpcKLHO_qNZWGi5HKYnfrrkd-sbZoA/viewform

I realize it's 5:45 a.m. in New Zealand, so this might end up waiting until after Maker Faire to get merged in. Sorry I didn't think to check the list of signers until now. In any case, comment here in the PR when you've signed. Thanks!

[towynlin]

Also, I like the idea of not instantiating Serial2 by default, since RAM optimization is moving quickly up the priority list.

[towynlin]

Sweet. Yes. Thank you. This looks great. Please make two changes for me:

• Do not instantiate Serial2 by default. We'll keep that 192 bytes of flash and 168 bytes of RAM available for most users. I really appreciate you doing the analysis there. Huge thanks! 💯
• Remove the README changes, and in the next few days, please submit some variation of them as a PR to the docs repo, including the clear instructions on how to instantiate Serial2.

It's OK for now to do require users to instantiate with USARTSerial Serial2(USART_D1_D0);. However, bonus points if you can make it more friendly to beginners only familiar with Arduino.

[zsup]

This might be a n00b suggestion, but just in case - would it be possible to have #include "Serial2.h" be the magic incantation? Or something along those lines. Would be a little friendlier.

[towynlin]

Absolutely possible. It doesn't fit with the naming of the rest of our file naming scheme—more consistent would be spark_wiring_serial2.h, but that's obviously less intuitive for users.

[zsup]

good point. I think in this case it’s worth diverging. We might also want to split libraries that aren’t included by default in a separate folder within the file structure, not unlike what Arduino does:

https://github.com/arduino/Arduino/tree/master/hardware/arduino/cores/arduino
https://github.com/arduino/Arduino/tree/master/libraries

That will make it easy for someone to discover the libraries that could be included through #include. This could also be an opportunity for RAM optimization for us (taking things that are currently always built in and pulling them out, requiring that they be included first)

On May 8, 2014 at 1:24:41 PM, Zachary Crockett (notifications@github.com) wrote:

Absolutely possible. It doesn't fit with the naming of the rest of our file naming scheme—more consistent would be spark_wiring_serial2.h, but that's obviously less intuitive for users.

—
Reply to this email directly or view it on GitHub.

[towynlin]

Hear, hear! @gruvin you have your task!

• create core-firmware/libraries/Serial2/Serial2.h
• add to the include path in the makefile so users can do #include "Serial2.h"

[gruvin]

OK, got all that. So an include file to instantiate Serial2 and the other housework ... and after Maker Faire. (Rome wasn't built in a day.) Thanks. :-)

Oh and, I just signed the agreement.

[gruvin]

Alas, I wasn't able to get to this before moving house (presently in progress) due to a melt down of a business server and the work that required. Might be another couple weeks.

[gruvin]

I've run into a bit of a snag. I wanted to have unneeded code excluded for the case when Serial2.h is not included from application.cpp -- things like the interrupt hander for USART1. Unfortunately though, files like spark_wiring_usartserial.cpp are compiled before application.cpp -- as indeed they must be. This means that Serial2.h has not been parsed before spark_wiring_usartserial.cpp, meaning we do not get the opportunity to exclude code using a simple #ifdef __LIB_SERIAL2_H, therein.

The only way forward then, given how I've implemented Serial2 by sharing the existing Serial driver class (most efficient method overall, I think) is to simply accept the additional overhead. :-/

Other data could have been saved, including the second record in the USART_Info array.

These two items amount to only a hand full of wasted 32-bit data words. But it irks me, none the less. Ordinarily, an unused C function would be excluded from the final output by the linker. But the interrupt function (ISR) is a special case. It will be included, even if it's not used, because its tied to an ISR vector. Luckily, all that function does is call the larger, 'real' ISR routine, passing in a single pointer. So not a lot of code. But still a little frustrating, not being able to remove it entirely (due to the approach I took earlier, pretty much.)

In any case, I will proceed as originally planned -- that being to keep what we have now and to simply separate instantiation of Serial2 off into its own libraries/Serial2.h file.

[gruvin]

Code size, as things stand right now ...

Without #include "Serial2.h" ...

arm-none-eabi-size --format=berkeley core-firmware.elf
   text    data     bss     dec     hex filename
  68732    2988   11604   83324   1457c core-firmware.elf

With #include "Serial2.h" ...

arm-none-eabi-size --format=berkeley core-firmware.elf
   text    data     bss     dec     hex filename
  68764    2988   11772   83524   14644 core-firmware.elf

[gruvin]

Oh dear. There seems to be a mix of _UMAP macro and myUSART style code in that last commit. I must have forgotten to revert some test code. That will need tidying up. D'oh.

[gruvin]

Another possible problem I just realised: The ISR function USART_Interrupt_Handler may not be re-entrant compliant. What if it is half way through processing an ISR for Serial1, then gets called for Serial2?

It looks OK to me ... but I'm very tired. Can I get a second opinion?

[andyw-lala]

Are interrupts reenabled ? If not then, I don't think reentrancy is an
issue. But commenting the code to highlight the susceptibility is probably
wise.
On May 28, 2014 4:21 AM, "Bryan" notifications@github.com wrote:

Another possible problem I just realised: The ISR function
USART_Interrupt_Handler may not be re-entrant compliant. What if it is
half way through processing an ISR for Serial1, then gets called for
Serial2?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/181#issuecomment-44382865
.

[gruvin]

@andyw-lala -- Thanks for that. I will add a comment, in case someone maybe wants to add static variables or something to the ISR some day, which would be one example of what could seriously break the shared function.

[gruvin]

Right. That last git push required figuring out how to recover from a, "detached HEAD". Whew!

Now I can go figure out how to edit Spark documentation. Any hints on that score welcomed! :-P

[gruvin]

Documentation pull request submitted: particle-iot/docs#160

This should mean we are ready to merge and roll.

[gruvin]

OK. Now we should be ready to merge and roll (after a final test by @towynlin, of course.

I need to get better organised or something. :-P