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
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -498,7 +498,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;

case HALF_PREC_FLOAT:
pDecodedItem->val.dfnum = IEEE754_HalfToDouble((uint16_t)uNumber);
pDecodedItem->uDataType = QCBOR_TYPE_DOUBLE;
Expand All @@ -511,6 +511,13 @@ inline static QCBORError DecodeSimple(uint8_t uAdditionalInfo, uint64_t uNumber,
pDecodedItem->val.dfnum = UsefulBufUtil_CopyUint64ToDouble(uNumber);
pDecodedItem->uDataType = QCBOR_TYPE_DOUBLE;
break;
#else
case HALF_PREC_FLOAT:
case SINGLE_PREC_FLOAT:
case DOUBLE_PREC_FLOAT:
nReturn = QCBOR_ERR_UNSUPPORTED;
break;
#endif // MBED_CONF_TARGET_ENABLE_FLOATING_POINT

case CBOR_SIMPLEV_FALSE: // 20
case CBOR_SIMPLEV_TRUE: // 21
Expand Down Expand Up @@ -633,6 +640,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

case QCBOR_TYPE_DOUBLE:
{
const double d = pDecodedItem->val.dfnum;
Expand All @@ -644,6 +652,12 @@ static int DecodeDateEpoch(QCBORItem *pDecodedItem)
pDecodedItem->val.epochDate.fSecondsFraction = d - pDecodedItem->val.epochDate.nSeconds;
}
break;
#else
case QCBOR_TYPE_DOUBLE:
nReturn = QCBOR_ERR_BAD_OPT_TAG;
goto Done;
break;
#endif // MBED_CONF_TARGET_ENABLE_FLOATING_POINT

default:
nReturn = QCBOR_ERR_BAD_OPT_TAG;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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?

// have to check float expected only to within an epsilon
int CHECK_EXPECTED_DOUBLE(double val, double expected) {

Expand All @@ -1513,6 +1513,7 @@ int CHECK_EXPECTED_DOUBLE(double val, double expected) {

return diff > 0.0000001;
}
#endif // MBED_CONF_TARGET_ENABLE_FLOATING_POINT


int DateParseTest()
Expand Down Expand Up @@ -1561,6 +1562,7 @@ int DateParseTest()
return -7;
}

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

If MBED_CONF_TARGET_ENABLE_FLOATING_POINT is false then you could check that the function returns the expected error code.
Did you run the tests to ensure that they pass with/without the compilation flag?

// Epoch date in float format with fractional seconds
if((nCBORError = QCBORDecode_GetNext(&DCtx, &Item)))
return -8;
Expand All @@ -1569,6 +1571,7 @@ int DateParseTest()
CHECK_EXPECTED_DOUBLE(Item.val.epochDate.fSecondsFraction, 0.1 )) {
return -9;
}
#endif // MBED_CONF_TARGET_ENABLE_FLOATING_POINT

// Epoch date float that is too large for our representation
if(QCBORDecode_GetNext(&DCtx, &Item) != QCBOR_ERR_DATE_OVERFLOW) {
Expand Down
15 changes: 15 additions & 0 deletions drivers/Timer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -75,10 +75,17 @@ int Timer::read_us()
return read_high_resolution_us();
}

#ifdef MBED_CONF_TARGET_ENABLE_FLOATING_POINT
float Timer::read()
{
return (float)read_high_resolution_us() / 1000000.0f;
}
#else
int Timer::read()
{
return read_high_resolution_us() / 1000000;
}
#endif // MBED_CONF_TARGET_ENABLE_FLOATING_POINT

int Timer::read_ms()
{
Expand Down Expand Up @@ -112,9 +119,17 @@ void Timer::reset()
core_util_critical_section_exit();
}


#ifdef MBED_CONF_TARGET_ENABLE_FLOATING_POINT
Timer::operator float()
{
return read();
}
#else
Timer::operator int()
{
return read();
}
#endif // MBED_CONF_TARGET_ENABLE_FLOATING_POINT

} // namespace mbed
8 changes: 8 additions & 0 deletions drivers/Timer.h
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,11 @@ class Timer : private NonCopyable<Timer> {
*
* @returns Time passed in seconds
*/
#ifdef MBED_CONF_TARGET_ENABLE_FLOATING_POINT
float read();
#else
int read();
#endif // MBED_CONF_TARGET_ENABLE_FLOATING_POINT

/** Get the time passed in milliseconds
*
Expand All @@ -90,7 +94,11 @@ class Timer : private NonCopyable<Timer> {

/** An operator shorthand for read()
*/
#ifdef MBED_CONF_TARGET_ENABLE_FLOATING_POINT
operator float();
#else
operator int();
#endif // 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 will have to add test coverage for this change.


/** Get in a high resolution type the time passed in microseconds.
* Returns a 64 bit integer.
Expand Down
4 changes: 3 additions & 1 deletion features/cellular/framework/AT/AT_CellularSMS.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1090,6 +1090,7 @@ AT_CellularSMS::sms_info_t *AT_CellularSMS::get_oldest_sms_index()
return retVal;
}

#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 am confused by this change becasue you are only compiling out the implementation of compare_time_strings. The function is called in get_oldest_sms_index. Either more functions should be compiled out or this function is not called by PDMC and should have been removed by the linker.

// if time_string_1 is greater (more fresh date) then return 1, same 0, smaller -1. Error -2
int AT_CellularSMS::compare_time_strings(const char *time_string_1, const char *time_string_2)
{
Expand All @@ -1113,6 +1114,7 @@ int AT_CellularSMS::compare_time_strings(const char *time_string_1, const char *

return retVal;
}
#endif // MBED_CONF_TARGET_ENABLE_FLOATING_POINT

bool AT_CellularSMS::create_time(const char *time_string, time_t *time)
{
Expand All @@ -1126,7 +1128,7 @@ bool AT_CellularSMS::create_time(const char *time_string, time_t *time)
&time_struct.tm_hour, &time_struct.tm_min, &time_struct.tm_sec, &sign, &gmt) == kNumberOfElements) {
*time = mktime(&time_struct);
// add timezone as seconds. gmt is in quarter of hours.
int x = 60 * 60 * gmt * 0.25;
int x = (60 / 4) * 60 * gmt;
if (sign == '+') {
*time += x;
} else {
Expand Down
5 changes: 5 additions & 0 deletions features/frameworks/unity/source/unity.c
Original file line number Diff line number Diff line change
Expand Up @@ -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

#define UNITY_EXCLUDE_DOUBLE
#define UNITY_EXCLUDE_FLOAT
#endif // MBED_CONF_TARGET_ENABLE_FLOATING_POINT

/* If omitted from header, declare overrideable prototypes here so they're ready for use */
#ifdef UNITY_OMIT_OUTPUT_CHAR_HEADER_DECLARATION
int UNITY_OUTPUT_CHAR(int);
Expand Down
68 changes: 61 additions & 7 deletions features/lorawan/lorastack/phy/LoRaPHY.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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.

int LoRaPHY::compute_symb_timeout_lora(uint8_t phy_dr, uint32_t bandwidth)
{
// in milliseconds
return ((float)(1 << phy_dr) / (float) bandwidth * 1000);
return ((int)(1 << phy_dr) / (int) bandwidth * 1000);
}

float LoRaPHY::compute_symb_timeout_fsk(uint8_t phy_dr)
int LoRaPHY::compute_symb_timeout_fsk(uint8_t phy_dr)
{
return (8.0f / (float) phy_dr); // 1 symbol equals 1 byte
return (8 / (int) phy_dr); // 1 symbol equals 1 byte
}


#ifdef MBED_CONF_TARGET_ENABLE_FLOATING_POINT
void LoRaPHY::get_rx_window_params(float t_symb, uint8_t min_rx_symb,
float error_fudge, float wakeup_time,
uint32_t *window_length, uint32_t *window_length_ms,
Expand Down Expand Up @@ -445,7 +445,51 @@ void LoRaPHY::get_rx_window_params(float t_symb, uint8_t min_rx_symb,
*window_length = (uint32_t) ceil(window_len_in_ms / t_symb);
*window_length_ms = window_len_in_ms;
}
#else
void LoRaPHY::get_rx_window_params(int t_symb, uint8_t min_rx_symb,
int error_fudge, int wakeup_time,
uint32_t *window_length, uint32_t *window_length_ms,
int32_t *window_offset,
uint8_t phy_dr)
{
int target_rx_window_offset;
int window_len_in_ms;

if (phy_params.fsk_supported && phy_dr == phy_params.max_rx_datarate) {
min_rx_symb = MAX_PREAMBLE_LENGTH;
}

// We wish to be as close as possible to the actual start of data, i.e.,
// we are interested in the preamble symbols which are at the tail of the
// preamble sequence.
target_rx_window_offset = (MAX_PREAMBLE_LENGTH - min_rx_symb) * t_symb; //in ms

// Actual window offset in ms in response to timing error fudge factor and
// radio wakeup/turned around time.
*window_offset = floor(target_rx_window_offset - error_fudge - wakeup_time);

// possible wait for next symbol start if we start inside the preamble
int possible_wait_for_symb_start = MIN(t_symb,
((2 * error_fudge) + wakeup_time + TICK_GRANULARITY_JITTER));

// how early we might start reception relative to transmit start (so negative if before transmit starts)
int earliest_possible_start_time = *window_offset - error_fudge - TICK_GRANULARITY_JITTER;

// time in (ms) we may have to wait for the other side to start transmission
int possible_wait_for_transmit = -earliest_possible_start_time;

// Minimum reception time plus extra time (in ms) we may have turned on before the
// other side started transmission
window_len_in_ms = (min_rx_symb * t_symb) + MAX(possible_wait_for_transmit, possible_wait_for_symb_start);

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

*window_length_ms = window_len_in_ms;
}
#endif // MBED_CONF_TARGET_ENABLE_FLOATING_POINT

#ifdef MBED_CONF_TARGET_ENABLE_FLOATING_POINT
int8_t LoRaPHY::compute_tx_power(int8_t tx_power_idx, float max_eirp,
float antenna_gain)
{
Expand All @@ -455,7 +499,17 @@ int8_t LoRaPHY::compute_tx_power(int8_t tx_power_idx, float max_eirp,

return phy_tx_power;
}
#else
int8_t LoRaPHY::compute_tx_power(int8_t tx_power_idx, int max_eirp,
int antenna_gain)
{
int8_t phy_tx_power = 0;

phy_tx_power = (int8_t) floor((max_eirp - (tx_power_idx * 2U)) - antenna_gain);

return phy_tx_power;
}
#endif // MBED_CONF_TARGET_ENABLE_FLOATING_POINT

int8_t LoRaPHY::get_next_lower_dr(int8_t dr, int8_t min_dr)
{
Expand Down Expand Up @@ -829,7 +883,7 @@ void LoRaPHY::compute_rx_win_params(int8_t datarate, uint8_t min_rx_symbols,
uint32_t rx_error,
rx_config_params_t *rx_conf_params)
{
float t_symbol = 0.0;
int t_symbol = 0;

// Get the datarate, perform a boundary check
rx_conf_params->datarate = MIN(datarate, phy_params.max_rx_datarate);
Expand All @@ -849,7 +903,7 @@ void LoRaPHY::compute_rx_win_params(int8_t datarate, uint8_t min_rx_symbols,
rx_conf_params->frequency = phy_params.channels.channel_list[rx_conf_params->channel].frequency;
}

get_rx_window_params(t_symbol, min_rx_symbols, (float) rx_error, MBED_CONF_LORA_WAKEUP_TIME,
get_rx_window_params(t_symbol, min_rx_symbols, (int) rx_error, MBED_CONF_LORA_WAKEUP_TIME,
&rx_conf_params->window_timeout, &rx_conf_params->window_timeout_ms,
&rx_conf_params->window_offset,
rx_conf_params->datarate);
Expand Down
16 changes: 14 additions & 2 deletions features/lorawan/lorastack/phy/LoRaPHY.h
Original file line number Diff line number Diff line change
Expand Up @@ -627,6 +627,7 @@ class LoRaPHY : private mbed::NonCopyable<LoRaPHY> {
uint8_t verify_link_ADR_req(verify_adr_params_t *verify_params, int8_t *dr,
int8_t *tx_pow, uint8_t *nb_rep);

#ifdef MBED_CONF_TARGET_ENABLE_FLOATING_POINT
/**
* Computes the RX window timeout and the RX window offset.
*/
Expand All @@ -635,11 +636,22 @@ class LoRaPHY : private mbed::NonCopyable<LoRaPHY> {
uint32_t *window_length, uint32_t *window_length_ms,
int32_t *window_offset,
uint8_t phy_dr);
#else
void get_rx_window_params(int t_symbol, uint8_t min_rx_symbols,
int rx_error, int wakeup_time,
uint32_t *window_length, uint32_t *window_length_ms,
int32_t *window_offset,
uint8_t phy_dr);
#endif // MBED_CONF_TARGET_ENABLE_FLOATING_POINT

#ifdef MBED_CONF_TARGET_ENABLE_FLOATING_POINT
/**
* Computes the txPower, based on the max EIRP and the antenna gain.
*/
int8_t compute_tx_power(int8_t txPowerIndex, float maxEirp, float antennaGain);
#else
int8_t compute_tx_power(int8_t txPowerIndex, int maxEirp, int antennaGain);
#endif // MBED_CONF_TARGET_ENABLE_FLOATING_POINT

/**
* Provides a random number in the range provided.
Expand Down Expand Up @@ -667,12 +679,12 @@ class LoRaPHY : private mbed::NonCopyable<LoRaPHY> {
/**
* Computes the symbol time for LoRa modulation.
*/
float compute_symb_timeout_lora(uint8_t phy_dr, uint32_t bandwidth);
int compute_symb_timeout_lora(uint8_t phy_dr, uint32_t bandwidth);

/**
* Computes the symbol time for FSK modulation.
*/
float compute_symb_timeout_fsk(uint8_t phy_dr);
int compute_symb_timeout_fsk(uint8_t phy_dr);

protected:
LoRaRadio *_radio;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.

static void thread_interface_bootsrap_mode_init(protocol_interface_info_entry_t *cur)
{
thread_routing_reset(&cur->thread_info->routing);
Expand Down Expand Up @@ -973,6 +974,7 @@ static void thread_interface_bootsrap_mode_init(protocol_interface_info_entry_t

cur->thread_info->thread_attached_state = THREAD_STATE_NETWORK_DISCOVER;
}
#endif // MBED_CONF_TARGET_ENABLE_FLOATING_POINT

int8_t thread_bootsrap_event_trig(thread_bootsrap_event_type_e event_type, int8_t Id, arm_library_event_priority_e priority)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -721,6 +721,7 @@ static void energy_scan_confirm_cb(int8_t if_id, const mlme_scan_conf_t *conf)
}
}

#ifdef MBED_CONF_TARGET_ENABLE_FLOATING_POINT
void thread_energy_scan_timeout_cb(void *arg)
{
link_configuration_s *linkConfiguration;
Expand Down Expand Up @@ -775,6 +776,7 @@ void thread_energy_scan_timeout_cb(void *arg)
s->mac_api->mlme_req(s->mac_api, MLME_SCAN, &req);
}
}
#endif // MBED_CONF_TARGET_ENABLE_FLOATING_POINT


static void thread_panid_scan_timeout_cb(void *arg)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,9 @@ static void thread_parse_accept(protocol_interface_info_entry_t *cur, mle_messag
static void thread_parse_annoucement(protocol_interface_info_entry_t *cur, mle_message_t *mle_msg);
static void thread_parse_data_response(protocol_interface_info_entry_t *cur, mle_message_t *mle_msg, uint8_t linkMargin);
static void thread_host_child_update_request_process(protocol_interface_info_entry_t *cur, mle_message_t *mle_msg, uint8_t linkMargin);
#ifdef MBED_CONF_TARGET_ENABLE_FLOATING_POINT
static void thread_parse_child_update_response(protocol_interface_info_entry_t *cur, mle_message_t *mle_msg, mle_security_header_t *security_headers, uint8_t linkMargin);
#endif // MBED_CONF_TARGET_ENABLE_FLOATING_POINT

/* Public functions */
void thread_general_mle_receive_cb(int8_t interface_id, mle_message_t *mle_msg, mle_security_header_t *security_headers)
Expand Down Expand Up @@ -824,6 +826,7 @@ static void thread_host_child_update_request_process(protocol_interface_info_ent
}
}

#ifdef MBED_CONF_TARGET_ENABLE_FLOATING_POINT
static void thread_parse_child_update_response(protocol_interface_info_entry_t *cur, mle_message_t *mle_msg, mle_security_header_t *security_headers, uint8_t linkMargin)
{
uint8_t mode;
Expand Down Expand Up @@ -907,5 +910,6 @@ static void thread_parse_child_update_response(protocol_interface_info_entry_t *
mac_data_poll_protocol_poll_mode_decrement(cur);

}
#endif // MBED_CONF_TARGET_ENABLE_FLOATING_POINT

#endif
Loading