Page 1 of 2

chTimeIsInRangeX always false?  Topic is solved

Posted: Fri Nov 22, 2019 2:08 pm
by wurstnase
Hi,

I've some issues with the following code:

Code: Select all

/**
 * @brief   Checks if the specified time is within the specified time range.
 * @note    When start==end then the function returns always true because the
 *          whole time range is specified.
 *
 * @param[in] time      the time to be verified
 * @param[in] start     the start of the time window (inclusive)
 * @param[in] end       the end of the time window (non inclusive)
 * @retval true         current time within the specified time window.
 * @retval false        current time not within the specified time window.
 *
 * @xclass
 */
static inline bool chTimeIsInRangeX(systime_t time,
                                    systime_t start,
                                    systime_t end) {

  return (bool)((systime_t)((systime_t)time - (systime_t)start) <
                (systime_t)((systime_t)end - (systime_t)start));
}

When end==start this returns always false, but in the comment it say always true.
When end==start this is always 0. time-start is always > 0, because it's unsigned. In that case 1 < 0 is always false?

In my current specific case:

Code: Select all

chTimeIsInRangeX(9682, 9659, 9659)

Re: chTimeIsInRangeX always false?

Posted: Fri Nov 22, 2019 2:17 pm
by Giovanni
Hi,

Moving in bug reports. I think the documentation is wrong in this case. This code changed a while ago but the documentation note persisted.

Making it as documented would probably require one extra check, it is better to keep this code efficient.

Giovanni

Re: chTimeIsInRangeX always false?

Posted: Fri Nov 22, 2019 7:23 pm
by wurstnase
If I'm correct I could now use in the fallback i2c driver?

Code: Select all

  i2cp->start = osalOsGetSystemTimeX();
  i2cp->end = i2cp->start + timeout;
  /* or i2cp->end = osalTimeAddX(i2cp->start, timeout); */

instead of this "wrong" code?

Code: Select all

  i2cp->start = osalOsGetSystemTimeX();
  i2cp->end = i2cp->start;
  if (timeout != TIME_INFINITE) {
    i2cp->end += timeout;
  }

Re: chTimeIsInRangeX always false?

Posted: Fri Nov 22, 2019 7:29 pm
by Giovanni
I need to review it, for sure it is not very correct to sum systime_t with sysinterval_t directly.

Giovanni

Re: chTimeIsInRangeX always false?

Posted: Fri Nov 22, 2019 8:03 pm
by wurstnase
Please check also my current patch. (Still testing)

Code: Select all

diff --git a/os/hal/lib/fallback/I2C/hal_i2c_lld.c b/os/hal/lib/fallback/I2C/hal_i2c_lld.c
index 1482d52f5..209506365 100644
--- a/os/hal/lib/fallback/I2C/hal_i2c_lld.c
+++ b/os/hal/lib/fallback/I2C/hal_i2c_lld.c
@@ -94,7 +94,7 @@ static inline msg_t i2c_check_arbitration(I2CDriver *i2cp) {
 
 static inline msg_t i2c_check_timeout(I2CDriver *i2cp) {
 
-  if (!osalOsIsTimeWithinX(osalOsGetSystemTimeX(), i2cp->start, i2cp->end)) {
+  if (!osalTimeIsInRangeX(osalOsGetSystemTimeX(), i2cp->start, i2cp->end)) {
     i2c_write_stop(i2cp);
     return MSG_TIMEOUT;
   }
@@ -105,7 +105,7 @@ static inline msg_t i2c_check_timeout(I2CDriver *i2cp) {
 static msg_t i2c_wait_clock(I2CDriver *i2cp) {
 
   while (palReadLine(i2cp->config->scl) == PAL_LOW) {
-    if (!osalOsIsTimeWithinX(osalOsGetSystemTimeX(), i2cp->start, i2cp->end)) {
+    if (!osalTimeIsInRangeX(osalOsGetSystemTimeX(), i2cp->start, i2cp->end)) {
       return MSG_TIMEOUT;
     }
     i2c_delay(i2cp);
@@ -347,14 +347,11 @@ void i2c_lld_stop(I2CDriver *i2cp) {
  */
 msg_t i2c_lld_master_receive_timeout(I2CDriver *i2cp, i2caddr_t addr,
                                      uint8_t *rxbuf, size_t rxbytes,
-                                     systime_t timeout) {
+                                     sysinterval_t timeout) {
 
   /* Setting timeout fields.*/
   i2cp->start = osalOsGetSystemTimeX();
-  i2cp->end = i2cp->start;
-  if (timeout != TIME_INFINITE) {
-    i2cp->end += timeout;
-  }
+  i2cp->end = osalTimeAddX(i2cp->start, timeout);
 
   CHECK_ERROR(i2c_write_start(i2cp));
 
@@ -399,14 +396,11 @@ msg_t i2c_lld_master_receive_timeout(I2CDriver *i2cp, i2caddr_t addr,
 msg_t i2c_lld_master_transmit_timeout(I2CDriver *i2cp, i2caddr_t addr,
                                       const uint8_t *txbuf, size_t txbytes,
                                       uint8_t *rxbuf, size_t rxbytes,
-                                      systime_t timeout) {
+                                      sysinterval_t timeout) {
 
   /* Setting timeout fields.*/
   i2cp->start = osalOsGetSystemTimeX();
-  i2cp->end = i2cp->start;
-  if (timeout != TIME_INFINITE) {
-    i2cp->end += timeout;
-  }
+  i2cp->end = osalTimeAddX(i2cp->start, timeout);
 
   /* send start condition */ 
   CHECK_ERROR(i2c_write_start(i2cp));
diff --git a/os/hal/lib/fallback/I2C/hal_i2c_lld.h b/os/hal/lib/fallback/I2C/hal_i2c_lld.h
index 21a7faabf..04614ece7 100644
--- a/os/hal/lib/fallback/I2C/hal_i2c_lld.h
+++ b/os/hal/lib/fallback/I2C/hal_i2c_lld.h
@@ -129,7 +129,7 @@ typedef struct {
   /**
    * @brief   Delay of an half bit time in system ticks.
    */
-  systime_t                 ticks;
+  sysinterval_t             ticks;
 #else
   /**
    * @brief   Pointer to an externally defined delay function.
@@ -217,10 +217,10 @@ extern "C" {
   msg_t i2c_lld_master_transmit_timeout(I2CDriver *i2cp, i2caddr_t addr,
                                         const uint8_t *txbuf, size_t txbytes,
                                         uint8_t *rxbuf, size_t rxbytes,
-                                        systime_t timeout);
+                                        sysinterval_t timeout);
   msg_t i2c_lld_master_receive_timeout(I2CDriver *i2cp, i2caddr_t addr,
                                        uint8_t *rxbuf, size_t rxbytes,
-                                       systime_t timeout);
+                                       sysinterval_t timeout);
 #ifdef __cplusplus
 }
 #endif


The default delay needs some rework, because it will trigger now a SV#4. The thdSleep is in a locked section.

Re: chTimeIsInRangeX always false?

Posted: Fri Nov 22, 2019 8:10 pm
by Giovanni
Thanks, I must have missed the fallback driver after changing the time API.

Giovanni

Re: chTimeIsInRangeX always false?

Posted: Sun Nov 24, 2019 5:08 pm
by Giovanni
Fixed as bugs #1060 and bug #1061 (trunk only right now).

The problem was present in both RT and NIL, also the OS-less OSAL contained a similar problem. Added specific tests to both test suites.

Do you mind giving a try to the modified I2C driver?

Giovanni

Re: chTimeIsInRangeX always false?

Posted: Mon Nov 25, 2019 10:03 am
by wurstnase
Hi Giovanni,

the driver is working now. Only the default delay is still running into a SV#4.

The i2cMasterTransmit call an osalSysLock.
Then it will start the transmit with i2c_lld_master_transmit_timeout.

E.g. in the i2c_write_start you will have:

Code: Select all

  palClearLine(i2cp->config->sda);
  i2c_delay(i2cp);
  palClearLine(i2cp->config->scl);


where the default i2c_delay is:

Code: Select all

static inline void i2c_delay(I2CDriver *i2cp) {

#if SW_I2C_USE_OSAL_DELAY || defined(__DOXYGEN__)
  osalThreadSleep(i2cp->config->ticks);
#else
  i2cp->config->delay();
#endif
}


Modifying the code to:

Code: Select all

static inline void i2c_delay(I2CDriver *i2cp) {

#if SW_I2C_USE_OSAL_DELAY || defined(__DOXYGEN__)
  osalSysUnlock();
  osalThreadSleep(i2cp->config->ticks);
  osalSysLock();
#else
  i2cp->config->delay();
#endif
}

Could fix it.

Re: chTimeIsInRangeX always false?

Posted: Thu Dec 05, 2019 3:45 pm
by Giovanni
Hi,

I applied a slightly different fix but your solution was correct.

Giovanni

Re: chTimeIsInRangeX always false?

Posted: Thu Dec 05, 2019 6:41 pm
by alex31
Giovanni wrote:Fixed as bugs #1060 and bug #1061 (trunk only right now).

The problem was present in both RT and NIL, also the OS-less OSAL contained a similar problem. Added specific tests to both test suites.

Do you mind giving a try to the modified I2C driver?

Giovanni



Hello,

Theses fixes have the side effect to make chThdSleepUntilWindowed(prev, next) to stuck if prev == next.

Alexandre