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

Feature multiplatform #24

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

Feature multiplatform #24

wants to merge 21 commits into from

Conversation

jy-wirepas
Copy link
Contributor

Add support for Darwin / macOS and Windows

Windows support is targeting Visual Studio 2015 or higher, so that the resulting binary is compatible with standard release of Python for Windows. This is required to create a Python wrapper for the C Mesh API library.

For the best user experience, typing "nmake" in the Microsoft Visual Studio
Developer Command Prompt should build the Wirepas C Mesh API library
successfully. The syntax of NMAKE and GNU make makefiles are sufficiently
different that a single makefile will not work for both.

This commit adds automatic detection of the make tool in use, and calls the
suitable tool-dependent makefile. Working NMAKE makefiles will be added in
another commit.
This commit refactors the platform-specific code and splits it in POSIX,
Linux and Darwin (macOS) parts. Both Linux and Darwin use the POSIX
functionality, requiring just a small amount of OS-dependent code.

Build support with Clang etc. on macOS is also implemented. The build
platform is auto-detected by the GNU make makefile.
As outlined in [CONTRIBUTING.md](CONTRIBUTING.md), path variables in
makefiles should not have a trailing path separator. This commit fixes
that.
Turns out that optimization was not turned on for the C Mesh API library.
Added the compiler flags to turn on optimizations.
The MSVC compiler does not understand the GCC __attribute__ syntax, but uses a
__pragma keyword instead. There's a pragma that can be used to enable struct
packing for a section of code instead of per type in the type definition,
like GCC does.

To support both GCC and MSVC in the same source code, an extra header file
called "compiler_ext.h" is introduced in this commit. It contains the
necessary macro trickery for managing packed structs.
The internal APIs of the SLIP module had some type inconsistencies and some
comments were mangled. This commit cleans up the module and also adds some
error checking that was missing.

Also, variable length arrays (VLAs) were turned into fixed size arrays, because
MSVC does not support VLAs.
…ility

Microsoft Visual Studio C compiler does not seem to like the multi-line
macros in msap.c and wpc.c. The macros have now been converted to static
inline functions.
When a packet is sent, an optional TX callback is called. There was a bug
in converting the buffering delay from an internal representation to
milliseconds. The TX result (sent / discarded) was also wrong.

This commit also fixes some unrelated comment spelling and formatting errors.
This commit adds NMAKE makefiles and a dummy versions of platform-specific
functions for Windows. This allows compiling the Wirepas C Mesh API library
on Windows, in Visual Studio Developer Command Prompt, by calling NMAKE.

Actual implementation of Windows-specific functions will be added in
another commit.
@PFigs
Copy link
Contributor

PFigs commented Jan 13, 2020

Are there any tests you could add to travis to keep mac specific code in check?

https://docs.travis-ci.com/user/multi-os/

@jy-wirepas
Copy link
Contributor Author

Are there any tests you could add to travis to keep mac specific code in check?

A test compile on a Mac would be useful. macOS has make, clang and other C toolchain components as part of the Xcode command line tools. No extra software is needed to compile the C Mesh API library on macOS.

Once I implement the Python wrapper, it needs a recent Python 3 install from Homebrew. There are a couple of modules from PyPi (Cython, setuptools) that are also needed.

@GwendalRaoul
Copy link
Contributor

Would it make sense to change the build system to use CMake instead?
I have never used it but seems to be the solution for what we plan to achieve.

@jy-wirepas
Copy link
Contributor Author

Would it make sense to change the build system to use CMake instead?
I have never used it but seems to be the solution for what we plan to achieve.

Perhaps. I've also not used CMake before. The C Mesh API library is simple enough that hand-written makefiles are feasible and that's why I did it for this feature, but at a later point we should probably consider adopting a better build system. Looks like CMake supports Visual Studio, Mac and Linux, so it seems like a good fit.

Some pointer parameters didn't have the const qualifier even though the
parameters were read-only. This commit adds the missing const qualifiers
to the public and internal APIs.

Please note: this commit changes the public API. The changes are mostly
backward compatible, but some callback prototypes have changed. As a result,
const qualifiers have to be added to any user-defined callback functions, or
else the code won't compile.
Some log message formatting and wording was inconsistent, so it was improved.
Also, a couple of potential bugs related to logging were fixed, as well as
some typos in comments.
Some error situations were not properly handled, so a couple of check were
added.
Even if the library file already exists, run make in the lib/ directory.
This ensures that any changes to the library are compiled as well.
This commit adds serial port, thread and logging support for Windows.
Add NMAKE makefiles and some small fixes to compile cleanly.
@jy-wirepas jy-wirepas marked this pull request as ready for review January 15, 2020 03:49
@jy-wirepas jy-wirepas self-assigned this Jan 15, 2020
@jy-wirepas jy-wirepas added the enhancement New feature or request label Jan 16, 2020
In order to use the Wirepas C Mesh API library in a Python extension module,
it has to be built with flags that are compatible with the official Python
release for Windows. The default flag /MT ("link with LIBCMT.LIB") needs to
be changed to /MD ("link with MSVCRT.LIB").
Move Windows toolchain settings in a separate makefile fragment and
harmonize the messages that are printed during the build for both
GNU and Windows platforms.
@PFigs
Copy link
Contributor

PFigs commented Jan 18, 2020

Codacy Here is an overview of what got changed by this pull request:

Issues
======
+ Solved 3
           

Clones added
============
- lib/platform/posix/logger.c  1
- lib/platform/win32/logger.c  1
- lib/platform/win32/platform.c  2
- lib/platform/posix/platform.c  2
         

See the complete overview on Codacy

@karl-dau
Copy link

Are there still plans to get multi platform support merged in?

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

Successfully merging this pull request may close these issues.

4 participants