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

Fix datatype issue for intervals #4

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jleguen
Copy link

@jleguen jleguen commented Aug 30, 2023

Issue

Using the LoRa Edge Config mobile application.
When trying to set static_scan_interval to 1440 minutes, the value was incorrectly clipped to 347.

Root causes

In tracker_utility.h, in the handling of BLE commands.

  1. Incorrect data type
    In SET_APP_STATIC_SCAN_INTERVAL_CMD, the values are extracted from a buffer of uint8_t to a temp uint16_t, which is too small for internal computation.
    Thus, the computed value in seconds is clipped, leading to an incorrect setting.
    The values stored in tracker_ctx are correctly of type uint32_t, but the clipping happens before.

    Example and compiler output:

    #include <assert.h>
    void test_casts(void) {
      uint8_t buffer[2] = {0x05, 0xA0};           // 05A0 == 1440 min == 86400 s
      uint16_t value = (uint16_t)buffer[0] << 8;  // Intermediate value
      value += buffer[1];
      value *= 60;  // XXX Result is too big for an uint16_t !
      assert(86400 == value);
    }
    In function ‘test_casts’:
    warning: comparison is always false due to limited range of data type [-Wtype-limits]
       76 |   assert(86400 == value);
          |                ^~
    
  2. Logic error
    Additionnaly, the comparison with STATIC_SCAN_INTERVAL_MIN and STATIC_SCAN_INTERVAL_MAX is directly done on the high byte of the raw payload buffer, instead of the final computed value (in seconds). See tracker_utility.c line 1685.

Fixes

This pull request fixes these two issues:

  1. Incorrect data type
    Use an uint32_t for the intermediate value used in computations.
  2. Logic error
    Use the computed value instead of the raw payload buffer.

@smtcbboulet
Copy link
Contributor

Thanks for the report Julien, the issue has been fixed in the next release.

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.

2 participants