-
Notifications
You must be signed in to change notification settings - Fork 73
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Various cleanups #23
base: master
Are you sure you want to change the base?
Various cleanups #23
Conversation
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.
I noticed I accidentially changed src/bin/bitlash in one commit, I force-pushed to correct that. I also added one more commit for the Makefile. About those binaries in the repository, do you really think they should be there? Seems to me like they're either always outdated, or you'll have to update them on every commit, which is cumbersome and enlarges the repository. Wouldn't it make more sense to remove them from the repository and separately offer them for download on your site? |
I added one more commit that fixes const-correctness. |
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.
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.
And I updated "Don't include Arduino.h in bitlash.cpp" to prevent it from breaking the Teensy 3.0 build. |
@@ -34,27 +34,16 @@ | |||
|
|||
|
|||
***/ | |||
#if defined(ARDUINO) && ARDUINO >= 100 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Support for Arduino pre-1.0 cannot be dropped, for backwards compatibility with existing projects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This particular change does not drop pre-1.0 support, it just removes these includes from bitlash.h in favor of src/bitlash.h (I was planning to drop pre-1.0 support for my upcoming pullrequest, but let's discuss this separately there).
Moving this discussion to the pullrequest instead of as a line note:
I actually think the commits in his pullrequest should not break the API or build settings at all, in the sense that everything that previously worked should still worked. Some things the previously failed (like disabling One thing that changed is that a UNIX_BUILD should now happen through running |
I think I answered all of your questions now. If I missed one, or there are other things, please let me know! :-) |
This pullrequest collects the various cleanup patches I collected while working on #20. These should be the easy ones to review and the shouldn't really change any code (though of course I haven't been able to test all of the possible environments).