Skip to content
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

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Various cleanups #23

wants to merge 13 commits into from

Conversation

matthijskooijman
Copy link

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).

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.
@matthijskooijman
Copy link
Author

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?

@matthijskooijman
Copy link
Author

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.
@matthijskooijman
Copy link
Author

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
Copy link
Owner

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.

Copy link
Author

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).

@matthijskooijman
Copy link
Author

Moving this discussion to the pullrequest instead of as a line note:

[const changes]
That’s cool. I am wondering if this will break the earlier versions of avr-gcc.
I don't suppose so, const annotations are part of the C++ standard and supported by gcc for a very long time. In any case, I've tested with Arduino 1.0.5 with avr-gcc 4.3.something. Are you interested in older versions too?

Would you please collect a list of API-breaking changes and build settings changes in your contribution so we have all of them on the table at once for discussion? Thanks!

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 SOFTWARE_SERIAL_TX without setting TINY_BUILD IIRC) now work as expected.

One thing that changed is that a UNIX_BUILD should now happen through running make in the src/ directory, running gcc -DUNIX_BUILD bitlash.cpp is no longer supported (since with my upcoming stream changes, additional options are needed and it would be cumbersome to have to pass all of those manually).

@matthijskooijman
Copy link
Author

I think I answered all of your questions now. If I missed one, or there are other things, please let me know! :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants