Conversation
bkueng
left a comment
bkueng
left a comment
There was a problem hiding this comment.
What I'm really wondering is what the define CUSTOM_FILE_IO is for... I couldn't find any documentation
Seems to be used to customize file IO in libc: https://chromium.googlesource.com/native_client/nacl-newlib/+/a9ae3c60b36dea3d8a10e18b1b6db952d21268c2/newlib/libc/include/sys/reent.h#228. But I don't know why we have it in our build.
There was a problem hiding this comment.
Itz is needed because:
- for example: https://github.com/PX4/Firmware/blob/fa5010109edf0c71914fc04bb2dc887918336f65/src/drivers/device/posix/cdev_platform.cpp#L334 uses
pthread_getname_npand also in other places of the codepthread_setname_npis used if I remember correctly. - Cygwin uses newlib c library and after clean installation in /usr/include/pthread.h there is this section:
/* Non posix calls */
#if __GNU_VISIBLE
int pthread_getattr_np (pthread_t, pthread_attr_t *);
int pthread_getname_np (pthread_t, char *, size_t) __attribute__((__nonnull__(2)));
int pthread_setname_np (pthread_t, const char *) __attribute__((__nonnull__(2)));
int pthread_sigqueue (pthread_t *, int, const union sigval);
int pthread_yield (void);
#endif
#if __MISC_VISIBLE /* HP-UX, others? */
int pthread_suspend (pthread_t);
int pthread_continue (pthread_t);
#endif
- I found out by reading this discussion: Cygwin build problem openwall/john#1250 (comment) that
_GNU_SOURCEis the define to use for enabling the build configuration that internally includes the define__GNU_VISIBLE - I added it
- It works
If you have any better solution please feel free to correct. Honestly I don't understand all the interna about these libraries and in my opinion it's so hard to find any documentation.
There was a problem hiding this comment.
Interesting and thanks for looking it up. I'm wondering why we don't need to define it then (or do I see that wrong)?
In any case I would try to reduce the scope to only the files where pthread_{g,s}etname_np is used. The define can lead to subtle changes in behavior.
Something like:
#define _GNU_SOURCE // for pthread_getname_np
#include <pthread.h>
#undef _GNU_SOURCE
There was a problem hiding this comment.
I see that _GNU_SOURCE is actually defined in our build, probably because we add -std=gnu++11.
So you can ignore my comment and it's ok to add the define globally (would be good to make this more explicit though for the other builds).
There was a problem hiding this comment.
Thanks for looking stuff up.
I see that _GNU_SOURCE is actually defined in our build
Why do I have to redefine it then? Is it dependent on the compiler? Does -std=gnu++11 just not get handeled in cygwin? That would explain it. In any case I don't see a risk for this define.
There was a problem hiding this comment.
Nitpick: it's implemented on NuttX too.
There was a problem hiding this comment.
Oh yeah, obviously you're right. I was thinking in the posix box but the message might be misleading.
EDIT: I corrected
|
Looking good. You effectively have 2 options for CI. For NuttX have you considered using the Windows version of the GCC ARM Embedded Toolchain natively? You could even use Visual Studio (project generated by cmake). |
|
@MaEtUgR I tried this - pretty cool. A few notes (that you already probably know)
Getting an ARM build working would be excellent. Getting an arm build + Gazebo would be the dogs bollocks. |
|
@hamishwillee Thanks for testing!
Totally agree. I will keep working on it 😉
|
In PX4 console I normally type Personally having jMAVSim working is fine for me - because mostly in docs I'm working in playing with parameters, and DroneCore. I know that ROS users and lots of our "future platform" work will find it essential. |
|
PS. Sander / Deltaquad also have a VM based simulator that is rather nice. It just does their vehicle, just does jMAVSim. What I like about it is that it has a nice GUI for starting/stopping refreshing, and presumably you could abstract this to easily do different vehicles. As a pure simulator on Windows it is "user friendly". Suffers from same stability problems as other VM based solutions and no UI of course. |
|
@MaEtUgR , I tried steps mentioned above and I ended up with the below error. Could you please guide me if I have missed anything? asheshad@BGL-ASHESHADX ~/Firmware It fails with the following output: Change Dir: /cygdrive/c/Work/Embedded_Targets/Work/Windows_PX4_cygwin_Matthias_grob/home/Firmware/build/posix_sitl_default/CMakeFiles/CMakeTmp Run Build Command:"/usr/bin/ninja.exe" "cmTC_457ec" [1/2] Building C object CMakeFiles/cmTC_457ec.dir/testCCompiler.c.o FAILED: CMakeFiles/cmTC_457ec.dir/testCCompiler.c.o /usr/bin/cc -o CMakeFiles/cmTC_457ec.dir/testCCompiler.c.o -c /bin/sh: /usr/bin/cc: cannot execute binary file: Exec format error ninja: build stopped: subcommand failed. CMake will not be able to correctly generate this project. -- Configuring incomplete, errors occurred! |
|
@AbhishekGS Looks to me like the toolcahin fails to execute ninja and the compiler...
I'm guessing here sorry, I don't really see the likely cause. |
I ran this under a differently named subfolder and it ran fine. So that is not the problem. I just did |
MaEtUgR
changed the title
|
I rebased on master and added the ARM NuttX on Cygwin support.
|
| add_custom_command(OUTPUT ${fw_io_bin} | ||
| COMMAND mkdir -p ${PX4_BINARY_DIR}/ROMFS/${config_romfs_root}/extras/ | ||
| COMMAND ${OBJCOPY} -O binary ${fw_io_exe} ${fw_io_bin} | ||
| COMMAND ${OBJCOPY} -O binary ${fw_io_exe_cyg} ${fw_io_bin_cyg} |
There was a problem hiding this comment.
This is kind of nitpicky, but instead of having special cygwin handling all over the place can we sidestep the issue by using relative paths?
https://github.com/PX4/Firmware/blob/master/platforms/nuttx/NuttX/CMakeLists.txt#L56-L57
| __END_DECLS | ||
|
|
||
| #elif defined(__PX4_LINUX) || defined(__PX4_NUTTX) || defined(__PX4_DARWIN) | ||
| #elif defined(__PX4_LINUX) || defined(__PX4_NUTTX) || defined(__PX4_DARWIN) || defined(__PX4_CYGWIN) |
There was a problem hiding this comment.
Move this to a single #else after QURT?
| #include <drivers/drv_hrt.h> | ||
|
|
||
| #if defined (__PX4_LINUX) || defined (__PX4_DARWIN) || defined(__PX4_QURT) | ||
| #if defined (__PX4_LINUX) || defined (__PX4_DARWIN) || defined(__PX4_CYGWIN) || defined(__PX4_QURT) |
There was a problem hiding this comment.
Flip this around to handle the special nuttx case first, otherwise use usleep.
@davids5 do you know why nuttx uses a delay instead of sleep here?
Ideally rc could be kept as a standalone lib without PX4 specifics.
There was a problem hiding this comment.
Probably because it's used from IRQ context. Then it also makes sense to have the ifdef's in the current way: if we were to add support for another RTOS, we would stumble upon this, and realize that usleep cannot be used.
There was a problem hiding this comment.
I agree, then it's more readable and the chances for a new platform which might be added is higher to succeed without error.
There was a problem hiding this comment.
Ah right, the bind is done in the px4io serial dma callback.
| else() | ||
| elseif(CYGWIN) | ||
| set(added_definitions | ||
| -D__PX4_POSIX |
There was a problem hiding this comment.
You can move -D__PX4_POSIX to the top of this file so that it's applied to all configs.
-Dnoreturn_function=__attribute__\(\(noreturn\)\) is also added for all posix configs.
There was a problem hiding this comment.
ok, let me see if there's an easy way.
|
@dagar In the latest commit I worked out a solution for all your comments except the first one. I'm pretty sure I break something if I do something like this: cygwin...cygwin-relative-paths-demo I didn't find a solution. But I'm also not completely sure why the conversion with the extra cygwin path is needed only in these places... |
|
Relative path usage update. cygwin...cygwin-relative-paths-demo |
|
@julianoes Thanks, I never built snapdragon until now otherwise I would have tried. I'm pretty sure it is not be affected. @dagar Your relative path solution is nice and works perfectly fine under cygwin. Thanks a lot for looking into it! I didn't know cmake has this feature. Now the paths in the By the way, here's the newest version of the toolchain which builds all this: |
|
OSX failure (circleci) is real. I'll look into it.
Update - the issue seems to be a difference in where Make vs Ninja perform the linking. |
|
@MaEtUgR I fixed the OSX makefile issue. Please test it under cygwin. |
|
Sadly the cygwin build is broken with these changes. Let me have a look. |
|
If I run it without the last commit I get the path With the last commit cmake writes the relative path EDIT: |
|
@dagar What do you think about the last commit? |
|
I rebased on master again and as discussed with @dagar reverted to using converted absolute paths to the linker script again because the ARM GCC linker seems to interprets relative paths differently on linux, mac and windows... Just for reference: |
|
Looks good. I strongly suggest having CI in place if you don't want this to break on a semi regular basis. PX4 Firmware CI
Toolchain update should be automated and compiler version needs to be kept in sync with Ubuntu (docker) and OSX. |
…nix-like environment Most of the incompatitbilities are luckily similar to the darwin build. - New target OS __PX4_CYGWIN added because in other build environments on Windows defines will very likely be completely different - added all necessary exeptions to the defines - disabled priorities completely because on Windows they are defined 1-32 and with all the arbitrary +40 -40 priority settings there were a lot of problems not only did some threads/"virtual tasks" not start because of out of bound priorities but also the resulting scheduling was totally random and inadequate with default priorities it ran toally fine during my first tests, should be rethought when windows is used onboard in the future
…t again because the relative path is interpreted differently on linux, mac and windows
|
Rebased to resolve merge conflicts. Let's get this in and continue iterating. |
7 participants
Most of the incompatitbilities are luckily similar to the darwin build.
__PX4_CYGWINadded because in other build environments on Windows defines will very likely be completely differentnot only did some threads/"virtual tasks" not start because of out of bound priorities but also the resulting scheduling was totally random and inadequate
with default priorities it ran toally fine during my first tests, should be rethought when windows is used onboard in the future
What I'm really wondering is what the define
__CUSTOM_FILE_IO__is for... I couldn't find any documentation. Any explanation appreciated.Here is an archive of the portable toolchain setup I tested with: https://drive.google.com/file/d/1y0SvGT6kPxJaB0T-qY2m0ONeR5ppzLGv/view?usp=sharing
If anyone wants to have a try just:
I will:
try to enable ARM build as wellEDIT: done