Switch to using Print and Stream classes internally #31
Conversation
|
Thank you. I will give the package a thorough review. Quick comment re: Pre-Arduino 1.0: One of the values I'd like the Bitlash project to exemplify is a commitment to backwards compatibility. In the embedded environment it's not surprising for a project to require maintenance many years after the initial build. Forcing a loyal and early-adopting Bitlash user to upgrade their toolchain to make a small change to their project, possibly to a toolchain and library set that contains additional years of space-gobbling but non-optional abstractions layered onto the core that will render their application un-buildable, is just something I am not willing to do if I can avoid it. If it compiled once on a toolset, we clearly have the code to make that work. Why throw it away? So, awkward as it may be, I'd like to keep the pre-1.0 build artifacts in place. Perhaps with a little thought you can make it less ugly and complex than it admittedly was. So, through this lens, even small changes like doCommand(const char *) that break existing code should be avoided, unless they provide definite value to the user of Bitlash. (PS: Disclaimer/acknowledgement: Bitlash 2.0 broke backwards compatibility in a big way to upgrade the language significantly. This is what I mean by user-visible benefits.) More later, and best regards, -br |
The setBaud function used by the "baud" function (e.g., called by func_setBaud) was only defined when SOFTWARE_SERIAL_TX, but the "baud" function was enabled whenever TINY_BUILD was not defined. This caused a compilation failure when SOFTWARE_SERIAL_TX was disabled but TINY_BUILD was not enabled.
That function will not be defined, causing a compiler error. To properly fix this, the USER_FUNCTIONS macro is moved from bitlash-functions.c to bitlash.h, so it can be used in bitlash-parser.c as well.
The previous macros were syntacticaly invalid and caused a compiler warning. Additionally, for targets that have GPIOR0 but not GPIOR1 they would cause compiler errors. This fixes both issues.
This is already done in bitlash.h, so this is not needed. However, in bitlash.h we do have to move up the Arduino.h include a bit, because the code that determines the build type depends on CORE_TEENSY, which is defined by Arduino.h when compiling for the Teensy.
Since there is a Makefile in src that just compiles the individual source files in src/ which does not need bitlash.cpp at all, it seems pointless to also support compiling the unix build by compiling bitlash.cpp manually.
In the Arduino build, they were being compiled as cpp already, since they were included by bitlash.cpp. This makes this fact more clear. In the UNIX build, they were compiled as .c, so this commit temporarily breaks the unix build.
This allows the Makefile to compile the code again, though it cannot be compiled correctly yet.
Now that we changed the filenames to .cpp, the UNIX_BUILD actually compiles them as C++ instead of C. This means we have to make sure that all functions are properly declared before being used (in C, using a function without a declaration would make the compiler just declare it on the spot, but this is not possible in C++ due to name mangling). Since the regular Arduino build already happened under C++, all the functions needed there were already declared. Additionally, this adds a "mode" parameter to the mkdir call, since that was left out (which didn't cause any compilation errors previously, but probably caused a random undefined value to be passed to mkdir).
Previously, UNIX_BUILD was automatically set, but this only worked on x86 and x86_64. Since the only point of the Makefile is to do UNIX builds, it might as well set the define explicitely, which could make the code work on other architectures out of the box as well.
Their implementations need to override the output function and cannot work without that.
|
I updated the https://github.com/Pinoccio/library-bitlash/tree/printstream-history branch to work with Arduino 012 and upwards (which I think is even more versions than before). I tested this with 013, 015, 019 and 023 (012 should work, but that download is broken on the Arduino site). Versions from 018 and upwards define On thing that is still unsolved is that the defines in |
|
Superb. Thank you for your careful testing on the many legacy versions. I’m interested to hear your thoughts for sorting out the remaining questions in bitlash.h and src/bitlash.h re: DEFAULT_CONSOLE_ONLY and the stream interface definitions. -br On Feb 7, 2014, at 4:06 AM, Matthijs Kooijman [email protected] wrote:
|
This moves more stuff into variables, which makes it easier to selectively change things in the build.
This fixes some compiler warnings that are shown for a UNIX_BUILD, since that build was also converted to C++, but also improves general const-correctness.
This code was broken and unused. If a console on software serial is needed again in the future, the upcoming changes should allow the Arduino SoftwareSerial library to be used.
It seems this relied on software serial reception, which was broken and removed. In the future, it should be possible to re-add support by just passing in a SoftwareSerial instance. Additionally, all ethernet examples now use the output handler feature instead of relying on the bitlash library itself to know how to talk to the Ethernet library or hardware.
In the future, it should be possible to just pass in an ethernet client Stream object and leave all the ethernet initialization stuff where it belongs. Additionally, all ethernet examples now use the output handler feature instead of relying on the bitlash library itself to know how to talk to the Ethernet library or hardware.
Now, src/bitlash.h requires that either ARDUINO or UNIX_BUILD is defined. If this is not the case, compilation fails with an error. Arduino versions below 018 did not define the ARDUINO variable. This means that for those versions, the user must manually set the Arduino version to make the build work. Furthermore, the ARDUINO_BUILD and ARDUINO_VERSION macros are dropped, since now the ARDUINO variable can just always be used.
Instead of keeping pin numbers and output handler functions and deciding on the fly where to send any printed bytes, now a Print* is kept, greatly simplifying sbp() and friends. Additionally, output can now also be redirected to an arbitrary Print object by calling setOutputHandler. Similarly, for the input a Stream* is kept. This allows users of the library to select any of the available serial port or any other Stream object to use as the main console. This allows for example to use an Ethernet client, without needing the bitlash library to understand every particular ethernet library available (as long as it implements the Stream interface). This change breaks compatibility with Arduino versions older than 012, since those do not have the Print class defined, making it non-trivial to support those. This also temporarily breaks compatibility with Arduino versions below 019, which don't contain the Stream class, but this will be fixed in a subsequent commit.
By defining DEFAULT_CONSOLE_ONLY, the initBitlash(Stream*) function is removed and the console is fixed to whatever DEFAULT_CONSOLE defines. Doing this allows the compiler to optimize away most of the overhead (like vtable lookups) involved. Arduino versions below 019 don't contain the Stream class, so for those versions DEFAULT_CONSOLE_ONLY is automatically defined to make those work again. However, since bitlash.h does not include src/bitlash.h, this does not work completely yet. Versions below 019 will still define try to define initBitlash(Stream&) in bitlash.h, breaking the build. This will be fixed by restructuring the include files in a few commits.
In these cases, either or both of the blout and bloutdefault global variables are not strictly required. By cleverly making these variables reference each other, the compiler can optimize them away, without having to change the code that uses these variables.
Since a few commits, bitlash does not support these versions anymore, so this code is no longer useful.
Wconstants only exists before Arduino 1.0 and includes only wiring.h, which is always included by WProgram.h already. Looking in the Arduino history, WConstants has always only included wiring.h. Furthermore, WProgram includes wiring.h since Arduino 012. Since we don't support Arduino < 012 anymore, we can drop this include without breaking anything.
This is possible since we now have a complete Stream object available and thus can just use the peek() method. Arduino versions below 1.0 did not have peek, so we resort to the old behaviour then.
The switch to using Stream and Print classes broke UNIX_BUILD, since no definitions of those classes were available. Instead of also adding those to bitlash, this switches to using the ArduinoUnix library and allows removing all of the unix-specific implementation of the Arduino API as well. This requires the ArduinoUnix library to be available (by default in a directory called `ArduinoUnix` besides the bitlash directory, but this can be changed by setting ARDUINO_UNIX_PATH).
This line checked for defined(ARDUINO) || defined(UNIX_BUILD), but that is guaranteed to be always true by a check further up.
The main goal of this commit is make the build options available to the public API, since not all functions are available with all options. This was previously not a problem for most functions (they would be declared but not defined, which is ok if they're not used either). However, on Arduino versions < 019, the Stream class is not available, so declaring initBitlash(Stream&) would break the build, even when it was not used. Fixing this is the original reason for this commit. This commit mostly moves declarations around, it should not make any functional changes. In addition to moving declarations between files, it also reorganizes the ordering of them and makes comments more consistent. The major changes in this commit are: - The public API used to be in bitlash.h, but is now moved to src/bitlash-public.h. bitlash.h still exists for sketches to include, but it just includes src/bitlash-public.h. - The private API and build config used to be in src/bitlash.h, but is now split into src/bitlash-private.h for the private API and src/bitlash-config.h for all build config related macros. src/bitlash.h no longer exists. - src/bitlash-public.h now includes src/bitlash-config.h, to allow the public API to be different depending on the build config. - A number of functions in the public API are now conditionally declared based on the build config. This matches the public API declarations to the actual function definitions in the .cpp files. - src/bitlash-private.h now includes src/bitlash-public.h. This allows removing duplicate declarations for the public API functions and types. - In the public API, the type of numvar now properly depends on TINY_BUILD. Before, the public API would always declare it as long, whereas the implementation would declare it as int for TINY_BUILD. This will probably have caused TINY_BUILD to be broken.
|
I updated the pullrequest once more, with a few minor fixes and the restructuring of header files I previously suggested. This should now allow the API to depend on the build config and in particular allow the build to really work again on older Arduino versions. See the printstream-history branch again for the changes since the last push (that I mostly squashed into earlier commits in this pullrequest). There's three more things to do:
|
|
Oh, one more important change: I previously said that Arduino < 015 was not supported by bitlash, but it turns out they were (I thought they weren't because However, Arduino versions < 012 do not define the Print class, making it non-trivial to keep supporting these with my changes. The above means that we're now dropping support for versions that previously worked. I hope this is ok for you? Note that Arduino version 012 and below do not seem to be available for download anymore either (links give 404) and that version 011 is now nearly 6 years old... |
|
Losing versions < 12 causes me no pain. They can go. -br On Feb 8, 2014, at 3:12 PM, Matthijs Kooijman [email protected] wrote:
|
Previously a mix of variables (pin number, override function) were used to determine where output should go to. These variables were checked whenever a byte was output to decide among a few possible output functions. The input stream to use was only selectable at compiletime, requiring changes to the library's files.
With these changes, the input and output are stored in a variable of type
Stream*andPrint*respectively. This allows users of the library to select the input stream to use from the sketch, at runtime, which makes things more flexible.This pullrequest is based on top of pullrequest #23, but I couldn't find a way to tell github this. This means that the commits from "Remove conioh.h and mygetch.c, these were unused" up to and including "Replace char * with const char * where applicable" should be ignored when reviewing this pullrequest. This pullrequest has a cleaned up history, the original commit sequence before I squashed things together is still available here: https://github.com/Pinoccio/library-bitlash/tree/printstream-history
Changes in the API:
initBitlash(Stream&)is added, to select a different stream to use as the main console. The existinginitBitlash(int baud)still works, and sets up bitlash to use the default console (usuall justSerial).setOutpuHandler(Print&)is added, to redirect output to an arbitraryPrintobject. Again, the existingsetOutputHandlerthat takes a function pointer still works.setOutputHandlerbeforeinitBitlash. If this is a problem, the old behaviour could be preserved, but requires a bit more complicated code due to optimizations.Serialobject, assuming theSerialobject transmits on pin 0), disregarding any output handler. Before, using pin number 0 would mean "don't redirect to a pin", so it would use the output handler. IMHO the new behaviour makes more sense, both in behaviour as well as making the code look more sane.initBitlashorsetOutputHandler.Furthermore, Arduino versions below 1.0 are no longer supported. I would say that 1.0 is old enough now (over 2 years) that almost all users will have upgraded and there is no point in supporting < 1.0 anymore. From your comments, it seems like you think otherwise. If you really think supporting older versions is necessary, we can see if we can do this, but it might require invasive changes that cancel some of the code clarity advantages of using the Stream and Print classes. In particular, it seems that the Stream class was only introduced in version 023, so just before 1.0. Perhaps we can get away by doing
#define Stream HardwareSerialfor older versions (and same for Print for even older versions).Another thing that is different pre-1.0 seems to be
.print()vs.write(). Apparently, pre-1.0 doing.print(uint8_t)wrote a raw single-byte instead of the number in ASCII digits. However, it seems that.write(uint8_t)has been available at least since 015, so it seems to me that just using that unconditionaly will work with version before and after 1.0 (or did they break this somewhere between 015 and 023, which I both checked?).Finally, I still need to test and possibly update the examples. I'll add that later, but feel free to already review these commits :-)