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

Remove floating point usage from PDMC #2

Closed
wants to merge 14 commits into from

Conversation

hugueskamba
Copy link
Owner

Description

This PR is not intended to be merged!! It only exists to allow Mbed-OS core team members to review prior to raising a public facing PR.

The changes remove the inclusion of __aeabi_xxxx symbols in the map file.
This is done using the following techniques:

  • Use a macro to exclude code that include floating point functions (of the form __aeabi_xxxx()).
  • Provide alternative Public and Protected API functions that return/accept integers. Private functions are simply changed to use integer instead of floating points.
  • Usage of literal floating point values have been replaced with integer values where it made sense.

PDMC builds successfully with the macro defined or not.

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[X] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

Release Notes

To fully remove the usage of floating point functions in Mbed OS when building PDMC, minimal printf usage of float also needs to be turned off in https://github.com/hugueskamba/mbed-os/blob/7d715ac05de43608692e6ce79374a553fa7ba24a/features/minimal-printf/mbed_lib.json or by overriding it in mbed_app.json.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Partially verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
We cannot verify signatures from co-authors, and some of the co-authors attributed to this commit require their commits to be signed.
@hugueskamba hugueskamba requested review from gpsimenos and evedon July 23, 2019 10:44
@hugueskamba hugueskamba reopened this Jul 30, 2019
Copy link
Collaborator

@evedon evedon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to understand more out to compile functionality out.
For changes to the network stack we have to liaise with the relevant team and let them handle the change.

@@ -201,7 +201,9 @@ static const uint16_t spBuiltInTagMap[] = {
CBOR_TAG_POS_BIGNUM, // See TAG_MAPPER_FIRST_FOUR
CBOR_TAG_NEG_BIGNUM, // See TAG_MAPPER_FIRST_FOUR
CBOR_TAG_FRACTION,
#ifdef MBED_CONF_TARGET_ENABLE_FLOATING_POINT
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The compilation flag is not required here. In fact it should not be used as the code compiled out is a list of built-in tags.

@@ -471,13 +473,15 @@ inline static QCBORError DecodeInteger(int nMajorType, uint64_t uNumber, QCBORIt
#error QCBOR_TYPE_BREAK macro value wrong
#endif

#ifdef MBED_CONF_TARGET_ENABLE_FLOATING_POINT
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no harm leaving these macros. I don't think we should use the compilation flag here.

@@ -498,7 +502,7 @@ inline static QCBORError DecodeSimple(uint8_t uAdditionalInfo, uint64_t uNumber,
case ADDINFO_RESERVED3: // 30
nReturn = QCBOR_ERR_UNSUPPORTED;
break;

#ifdef MBED_CONF_TARGET_ENABLE_FLOATING_POINT
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the code should be

case HALF_PREC_FLOAT:
#ifdef MBED_CONF_TARGET_ENABLE_FLOATING_POINT
    pDecodedItem->val.dfnum = IEEE754_HalfToDouble((uint16_t)uNumber);
    pDecodedItem->uDataType = QCBOR_TYPE_DOUBLE;
#else
    mbed_error( ... );
    nReturn = QCBOR_ERR_UNSUPPORTED;
#endif
    break;

@@ -633,6 +638,7 @@ static int DecodeDateEpoch(QCBORItem *pDecodedItem)
pDecodedItem->val.epochDate.nSeconds = pDecodedItem->val.uint64;
break;

#ifdef MBED_CONF_TARGET_ENABLE_FLOATING_POINT
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly:

case QCBOR_TYPE_DOUBLE:
#ifdef MBED_CONF_TARGET_ENABLE_FLOATING_POINT
   
#else
    mbed_error();
    nReturn = QCBOR_ERR_BAD_OPT_TAG;
    goto Done;
#endif

@@ -1503,7 +1503,7 @@ static uint8_t spDateTestInput[] = {

};


#ifdef MBED_CONF_TARGET_ENABLE_FLOATING_POINT
Copy link
Collaborator

@evedon evedon Jul 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no harm leaving this code in a test file. I prefer to limit the usage of this flag to the minimum necessary.
Why a test file is included in the build?

@@ -8,6 +8,11 @@
#include "utest/unity_handler.h"
#include <stddef.h>

#ifndef MBED_CONF_TARGET_ENABLE_FLOATING_POINT
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should exclude float/double in unity

@@ -392,18 +392,18 @@ uint8_t LoRaPHY::verify_link_ADR_req(verify_adr_params_t *verify_params,
return status;
}

float LoRaPHY::compute_symb_timeout_lora(uint8_t phy_dr, uint32_t bandwidth)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we can change floats for integers like that. This is a significant change that needs to be done by the team responsible for lora stack.
We could only compile out part of the stack if this is not core functionality.


// Setting the window_length in terms of 'symbols' for LoRa modulation or
// in terms of 'bytes' for FSK
*window_length = (uint32_t) ceil(window_len_in_ms / t_symb);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

floor and ceil take floats / doubles as arguments / return value so I am not sure we have removed floating point usage. In any case, as per previous comment, I don't think we can do that.

@@ -923,6 +923,7 @@ void thread_interface_init(protocol_interface_info_entry_t *cur)

}

#ifdef MBED_CONF_TARGET_ENABLE_FLOATING_POINT
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to understand more what the impact of removing functions that use floating points is.

@@ -51,6 +51,10 @@
"init-us-ticker-at-boot": {
"help": "Initialize the microsecond ticker at boot rather than on first use, and leave it initialized. This speeds up wait_us in particular.",
"value": false
},
"enable-floating-point": {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this parameter belong in targets.json. I would define it as a config parameter in mbed-os/platform/mbed_lib.json instead.

@hugueskamba hugueskamba deleted the hk-mbed-printf branch November 18, 2019 15:09
hugueskamba pushed a commit that referenced this pull request Feb 5, 2020
Signed-off-by: PARKJIHOON <jh6186.park@samsung.com>
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

2 participants