Changed CircBuffer to take its size as a template parameters for efficientcy.

[odinthenerd]

This results in smaller code size and less ram usage because the optimizer knows the size at compile time and it does not need to be stored in ram. It should also be faster because there is no indirection to the heap.

I have not tested this code!

[0xc0170]

PLease look at Travis, it fails with errors

[odinthenerd]

Yes sorry my bad, I overlooked the no c++11 requirement #newbieerror I'll fix it and resubmit.

[odinthenerd]

I tested this using the compiler here: https://developer.mbed.org/compiler/ with a LPC1549 expresso v2 board, should work this time

[odinthenerd]

On a side note: the current USBSerial implementation has some race conditions (if USBSerial::availible() gets interrupted by a USBSerial::EPBULK_OUT_callback for example). Should I file a new issue? What level of professionalism are you aiming at in this repo?

[odinthenerd]

just signed http://developer.mbed.org/contributor_agreement/ (#newbieerror number 2)

[Sissors]

It is well known that I am always big on efficiency. But is with a reasonable buffer size this really a significant deal memory wise? Not that it hurts to do this, but still.

Regarding the possible race condition, seems you are correct yes. It could happen there, and should be fixed. Although it requires a really specific situation where it would actually matter.

[odinthenerd]

Saves 6 bytes (pointer plus size variable) so 5%, not much, however I would expect that the heap will add some padding although I must admit I haven't looked at newlib-nanno's heap implementation. It would help with heap fragmentation to have less chunks on the heap though.

It certainly would help code size, although there are a lot of things that would help with code size here, I would suggest using static polymorphism (using CRTP) rather than virtual functions to have base code call member functions from the derived class. Also since there cannot be more than one instance of the usb class any way I would make all member functions and member variables static, this helps the optimizer in a huge way and also lowers the depth of the call tree which could lower the max stack depth which is again a ram savings. I can't show the code but we implemented a CDC on an LPC1769 in 2.5kb of flash, your implementation takes over 15k, there is a lot of potential there.

Code size is not so important in its self but it is a rough metric for how long execution time will be which again is a power consumption issue. If you decrease the length of the ISR handler chances are the processor will spend the freed up time sleeping.

Anyway back on topic I don't really see a disadvantage in changing size to a compile time parameter.

• you already used templates rather than hard coding uint8_t as the data type so no C like syntax
• the proposed style of syntax is already known by C++ programmers from std::array
• I can't imagine a scenario where the size would not be known at compile time
• the current implementation of USBSerial it is hard coded and no one seems to be complaining

[odinthenerd]

Regarding the race conditions, in case someone gets around to it before I do; since loads and stores are atomic this should work fine.

I found a few more race conditions, like issue #1049 or #1056. Using bit banding should fix that easily. Before reviewing more code, are there guidelines for how the users 'should' use this library regarding race conditions? I am assuming calling init functions from an IRQ is illegal or else you have race conditions in virtually every module (on power clock register for example), is it documented somewhere that you are not allowed to do this?

I have since found the "Before pull request checklist" here, sorry for not doing this right the first time.

[adamgreen]

The code used to malloc the equivalent of Size+1. It seems like it should be "T buf[Size+1];" here. Wouldn't the current code allow the read/write members to overflow the buf array by 1 element?

[odinthenerd]

good catch! It should be T buf[size] (with a small s!), stupid error on my part. As I mentioned somewhere else this circular buffer also has a bunch of race conditions. I have not had the time to do a re write yet, if you are interested since loads and stores are atomic on cortex this should work fine if adapted.

[0xc0170]

+1 adam. @porkybrain please send PR with a fix

[odinthenerd]

done #1161

[adamgreen]

Thanks for fixing.

[odinthenerd]

Lol more like sorry for breaking it