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

Use public domain half<->float routines which dependence on math functions in C stdlib #60

Merged
merged 8 commits into from
Aug 24, 2015

Conversation

bendyer
Copy link

@bendyer bendyer commented Aug 21, 2015

No description provided.

@pavel-kirienko
Copy link
Member

Thanks Ben. Please apply uncrustify to the modified file.

Have you run unit tests?

@bendyer
Copy link
Author

bendyer commented Aug 21, 2015

The unit tests complete successfully; I can add extra conversion and round-trip tests if you like?

@pavel-kirienko
Copy link
Member

No, the existing tests should be enough I guess.

I just noticed that these new routines assume that the native representation of float is IEEE754 single precision, hence they won't work on platforms where a different representation is used.

The original routines were taken from the half library, and they were supposedly compatible with non-IEEE representations, but since I don't have any non-IEEE hardware, I weren't able to actually verify that.

The question is: shall we sacrifice the non-confirmed compatibility with non-IEEE floats for the sake of less dependency on the C library?

Could you estimate the difference in image sizes between these two approaches? You said about 5-10 KB, which is quite significant, but a more precise result would be welcome.

@LorenzMeier
Copy link

I would propose to throw a compile error on non-IEEE floating point units. I don't think that they really exist and if yes then someone should make the effort and contribute the required code to deal with them.

@pavel-kirienko
Copy link
Member

Fair enough, although I saw IEEE754 check based on std::numeric_limits<>::is_iec559 to malfunction under IAR ARM compiler.

Let's wait for estimates from Ben.

@bendyer
Copy link
Author

bendyer commented Aug 21, 2015

5–10 KiB was an overall figure including the global data type registry changes—this is the less significant of the two.

Difference in size of my airspeed sensor application, compiled with:

$ /usr/local/bin/arm-none-eabi-gcc --version
arm-none-eabi-gcc (GNU Tools for ARM Embedded Processors) 4.8.4 20140725 (release) [ARM/embedded-4_8-branch revision 213147]
Copyright (C) 2013 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
Optimization flags New size (B) Current size (B) Size reduction (B)
-Os 11760 13420 1660
-O3 15556 17224 1668
-O2 13292 14968 1676
-O1 13404 15064 1660
-O0 36800 38836 2036

By comparison, disabling the global data type registry saves, on a fairly minimal node, 3372 bytes at -O3 and 5632 bytes at -O0 assuming you stub out most of the C++ standard library calls (and a fair bit more if you don't).

@pavel-kirienko
Copy link
Member

@bendyer please review my changes.

@antoinealb
Copy link

As a side note the full license (or a link to it) should be added in addition to the "BSD license" remark: https://github.com/numpy/numpy/blob/master/LICENSE.txt

@pavel-kirienko
Copy link
Member

@antoinealb
Copy link

No, according to the BSD license, reused code must keep the original full license (see above link). Now I don't think we might have problems because of this, but is is just good etiquette.

@pavel-kirienko
Copy link
Member

Ok, if my understanding is correct, we have two options:

  • Numpy folks allow us to use this code as-is, without inclusing full license, essentially allowing us to redistribute their code under the MIT license.
  • We don't use their code at all.

Including the BSD license text into our sources is not an option, because it will impose restrictions of the BSD license on the entire project. See http://programmers.stackexchange.com/a/122008.

@antoinealb
Copy link

Good catch, I thought the two licenses were compatible.

Relevant paper if we decide not to use their code: ftp://ftp.fox-toolkit.org/pub/fasthalffloatconversion.pdf

@bendyer
Copy link
Author

bendyer commented Aug 21, 2015

The NumPy version of the BSD license is two-clause + no-endorsement, so I don't think that StackExchange answer is applicable in this case. The only functional difference between that license and MIT is the no-endorsement clause, which is "Neither the name of the NumPy Developers nor the names of any contributors may be used to endorse or promote products derived from this software without specific prior written permission."

@pavel-kirienko
Copy link
Member

This is not the only difference that matters. The NumPy's license has a requirement

Redistributions in binary form must reproduce the above copyright notice, this list of conditions and the following disclaimer in the documentation and/or other materials provided with the distribution.

which will not be met under current terms of libuavcan distribution.

I guess the easiest solution is just to ask them for an exception for these two functions.

@bendyer
Copy link
Author

bendyer commented Aug 21, 2015

The MIT license used by libuavcan has exactly the same requirement for attribution:

The above copyright notice and this permission notice shall be included in all copies or substantial portions of the Software.

@pavel-kirienko
Copy link
Member

I'm not talking about attribution. The "above copyright notice" and list of conditions that NumPy license refers to are not the same as in libuavcan's license, therefore terms will not be met.

@bendyer
Copy link
Author

bendyer commented Aug 21, 2015

That position is incompatible with the inclusion of any non-public-domain code in libuavcan, including the existing half code, since the "above copyright notice" for e.g. the half library is "Copyright (c) 2012-2013 Christian Rau".

I don't think it's a big deal to include licenses for other code as required in the top-level LICENSE file, but if you want the license to stay exactly as-is then this code will need to be replaced with a public-domain or from-scratch implementation.

@pavel-kirienko
Copy link
Member

It is probably true that trying stay strict about licensing inevitably gets ridiculous at some point. Its bothering me that the BSD license, unlike MIT, has requirements concerning binary distributions (see the quote above), the question then is how seriously shall we take it.

@LorenzMeier, could you provide an advice here?

@bendyer
Copy link
Author

bendyer commented Aug 21, 2015

Generally speaking, the MIT license is considered to have requirements concerning binary distributions as well—while it hasn't been tested in court (as far as I'm aware), "Software" in the context of the license clearly refers to both source and compiled forms.

By way of example, the license section on Apple products contains full license information for all software used by the device, including many examples of 2-clause BSD, 3-clause BSD, ISC and MIT licenses. it's also common to see commercial software incorporating MIT-licensed components display the licenses for those components in appropriate locations.

@pavel-kirienko
Copy link
Member

Let me put the question this way: Can we use these two routines without including the full text of the BSD license? This would correspond to the first option above.

@bendyer
Copy link
Author

bendyer commented Aug 22, 2015

No. This code as well as the bxCAN driver definitions (https://github.com/UAVCAN/libuavcan/blob/master/libuavcan_drivers/stm32/driver/include/uavcan_stm32/bxcan.hpp#L3) would both require the appropriate BSD license to be included in the license file.

I think the licensing objection to the code in this PR is inconsistent with existing usage of MIT- and BSD-licensed code elsewhere in the project, but assuming your position is to grandfather existing code in and require all new code to be public domain, I'll replace this with a public domain equivalent.

@bendyer
Copy link
Author

bendyer commented Aug 22, 2015

What's your position on architecture-specific optimizations enabled via config flags, e.g. a flag to use VCVTB and VCVTT on Cortex-M4F devices?

@pavel-kirienko
Copy link
Member

I think the licensing objection to the code in this PR is inconsistent with existing usage of MIT- and BSD-licensed code elsewhere in the project

True, the inconsistency is caused by mere negligence on my part about borrowing 3rd party code. I'm not randomly picking on your contribution, it's just happened to reveal an existing problem which should be resolved separately.

I would like to keep licensing terms as simple as possible, and in this regard the MIT license is superior to the 3-clause BSD that NumPy is using. At the moment, the library contains the following borrowed pieces of code:

  • Your float16 conversion routines (I'm considering the version from half already replaced).
  • Bit array copy routine, which happens to be released under cc-wiki, which turned out to be contaminating. It must be either removed, or the original author should somehow re-license it.
  • The bxCAN definitions you mentioned, and also timer register definitions that @davids5 is going to bring in with one of his upcoming contributions. Being just a basic set of definitions, they probably could be left without attribution to NuttX. Even if not, it is not part of the core, so we could apply a separate license to the STM32 driver.

If there's an option to avoid copying non public domain 3rd party code into the core codebase, we should use it; so if you can replace the float16 routines with a public domain code, as you said, then please replace it.

What's your position on architecture-specific optimizations enabled via config flags, e.g. a flag to use VCVTB and VCVTT on Cortex-M4F devices?

This is a good idea. I propose to add a build config, say UAVCAN_USE_EXTERNAL_FLOAT16_CONVERSION (for a want of better name), which will be ifdefing out the definitions in uc_float_spec.cpp, allowing the platofrm driver or the application to provide custom definitions. A similar approach is used for snprintf.

@LorenzMeier
Copy link

On your question on licensing: The choice of 3-clause BSD was a very conscious one on the side of PX4. We wanted to meet these goals:

  • Share based on the insight that its mutually beneficial and not forcing people into a particular model
  • Have a license that allows inclusion of the project in proprietary, GPL and Apache codebases
  • Still retain attribution. The intent is to allow the project from benefiting of adoption in larger systems at least on the PR / developer recruiting side. Without this term there would be no way to track adoption in the proprietary space
  • Ensure that if questionable hardware is marketed by using our codebase we have legal means of stepping in

The last point turned out so far to not be relevant, so 2-clause BSD might have suited our goals as well. I think MIT is a good choice, however, if it becomes too limiting I would recommend to do a LICENSE.md file which details the used licenses, as long as what they require is not more than attribution.

@bendyer bendyer changed the title Use NumPy half<->float routines to avoid dependence on math functions in C stdlib Use public domain half<->float routines which dependence on math functions in C stdlib Aug 24, 2015
@bendyer
Copy link
Author

bendyer commented Aug 24, 2015

I have replaced the NumPy code with a public domain equivalent, which also passes all unit tests. This code is shorter than the NumPy version, and on my test project reduces the compiled size at -O3 by 88 bytes and at -Os by 56 bytes. A single floating-point operation (multiply or add) is used in each conversion, which would trigger the inclusion of the soft-FP code on processors with no FPU and potentially negate the size advantage—however I'm having trouble imagining a scenario where half<->float conversion is required, but the multiply and/or add soft-FP routines are not already linked.

I have also added the UAVCAN_USE_EXTERNAL_FLOAT16_CONVERSION flag to allow external implementations. Using the Cortex-M4F intrinsic in place of these functions saves 136 bytes at -Os (cumulative with the above reduction).

pavel-kirienko added a commit that referenced this pull request Aug 24, 2015
Use public domain half<->float routines which dependence on math functions in C stdlib
@pavel-kirienko pavel-kirienko merged commit be7f687 into master Aug 24, 2015
@pavel-kirienko
Copy link
Member

Thanks Ben!

@pavel-kirienko pavel-kirienko deleted the bendyer-half-nostdlib branch August 24, 2015 11:18
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.

None yet

4 participants