Doubt regarding drivers implementation

Discussions and support about ChibiOS/HAL, the MCU Hardware Abstraction Layer.
0x3333
Posts: 57
Joined: Thu Mar 07, 2019 10:19 pm
Has thanked: 7 times
Been thanked: 6 times

Re: Doubt regarding drivers implementation

Postby 0x3333 » Mon Aug 05, 2019 8:09 pm

Wow, that was fast!

1) I'm not sure if I'm using any ch function, as long as I can tell, I'm only using chnAddFlagsI that starts with "ch".
2) I followed the i2c fallback as example, it has some instances defined, so I just believed it was the norm. Removed.

Code: Select all

/** @brief I2C1 driver identifier.*/
#if SW_I2C_USE_I2C1 || defined(__DOXYGEN__)
I2CDriver I2CD1;
#endif

3) I did regarding IO lines(IO Lines Config), the GPT driver itself is a #define(GPT Driver define), because the same GPT is used across all instances, so no need to define it in every instance.
4) I'll work on that, is that OK to create a function to be called every "tick"? In my driver, a tick is 3 times de baud rate. I used it that way because I thought that it would be too cumbersome to the user to do that, but sounds more flexible that way.
5) The static variable is because I was using a single timer to all instances, so I needed to know when the last one called "stop" so I could stop the GPT. I'll work based on #4.

Will refactor things and let you know.

0x3333
Posts: 57
Joined: Thu Mar 07, 2019 10:19 pm
Has thanked: 7 times
Been thanked: 6 times

Re: Doubt regarding drivers implementation

Postby 0x3333 » Mon Aug 05, 2019 8:15 pm

To detect start bit, I'm using PAL event, should this be delegated to the user too?

Code: Select all

palEnableLineEvent(ssdp->config->rx, PAL_EVENT_MODE_FALLING_EDGE);
palSetLineCallback(ssdp->config->rx, (palcallback_t)_pal_interrupt, ssdp);

User avatar
Giovanni
Site Admin
Posts: 12177
Joined: Wed May 27, 2009 8:48 am
Location: Salerno, Italy
Has thanked: 570 times
Been thanked: 487 times
Contact:

Re: Doubt regarding drivers implementation

Postby Giovanni » Mon Aug 05, 2019 8:24 pm

I would consider another approach: do not rely on fronts on RX data but implement a state machine for the RX part.

Set up a clock x4 or x8 of the desired baud rate then design a state machine for frame decoding, your start would be the first RX read as zero while in the IDLE initial state. Using a state machine you just need the clock and and RX/TX lines. It would also allow to detect false starts, framing problems etc. Maybe this is a little bit more complex than just sampling at intervals.

OK, stopping here :) lets see the finished thing.

Giovanni

0x3333
Posts: 57
Joined: Thu Mar 07, 2019 10:19 pm
Has thanked: 7 times
Been thanked: 6 times

Re: Doubt regarding drivers implementation

Postby 0x3333 » Mon Aug 05, 2019 8:38 pm

Well, that's what I did :)

This function(State machine) is called every GPT tick.

Code: Select all


typedef enum {
  SSD_RX_STOP = 0,                   /**< Stopped.                           */
  SSD_RX_WAIT_START = 1,             /**< Waiting start bit.                 */
  SSD_RX_BIT = 2,                    /**< Receiving bit.                     */
  SSD_RX_WAIT_STOP = 3               /**< Waiting stop bit.                  */
} ssdrxstate_t;

static inline void _process_state(SoftSerialDriver * ssdp) {

  // RX
  if (ssdp->rx.next_counter == counter)
  {
    osalSysLockFromISR();
    switch (ssdp->rx.state) {
    case SSD_RX_BIT:
      ssdp->rx.byte >>= 1;
      if (palReadLine(ssdp->config->rx))
        ssdp->rx.byte |= 0x80;
      ssdp->rx.next_counter = counter + SOFT_SERIAL_BITRATE_MULTIPLIER;
      ssdp->rx.byte_index++;
      if (ssdp->rx.byte_index == 8)
        ssdp->rx.state = SSD_RX_WAIT_STOP;
      break;

    case SSD_RX_WAIT_STOP:
      osalSysLockFromISR();
      if (palReadLine(ssdp->config->rx))
        // If Stop bit, accept byte
        ssdIncomingDataI(ssdp, ssdp->rx.byte);
      else
        // If not consider a frame error, ignore byte
        chnAddFlagsI(ssdp, SSD_FRAMING_ERROR);
      ssdp->rx.state = SSD_RX_WAIT_START;
      osalSysUnlockFromISR();
      break;

    case SSD_RX_STOP:
    case SSD_RX_WAIT_START:
    default:
      break;
    }
    osalSysUnlockFromISR();
  }
  // TX
  if (ssdp->tx.next_counter == counter)
  {
    osalSysLockFromISR();
    msg_t b;
    switch (ssdp->tx.state) {
    case SSD_TX_START_BIT:
      b = ssdRequestDataI(ssdp);
      if (b < MSG_OK) {
        ssdp->tx.state = SSD_TX_STOP;
      } else {
        ssdp->tx.byte = b;
        ssdp->tx.byte_index = 0;
        palClearLine(ssdp->config->tx);
        ssdp->tx.next_counter = counter + SOFT_SERIAL_BITRATE_MULTIPLIER;
        ssdp->tx.state = SSD_TX_BIT;
      }
      break;

    case SSD_TX_BIT:
      palWriteLine(ssdp->config->tx, ssdp->tx.byte & 1);
      ssdp->tx.byte >>= 1;
      ssdp->tx.byte_index++;
      if (ssdp->rx.byte_index == 8) {
        ssdp->tx.state = SSD_TX_STOP_BIT;
      }
      ssdp->tx.next_counter = counter + SOFT_SERIAL_BITRATE_MULTIPLIER;
      break;

    case SSD_TX_STOP_BIT:
      palSetLine(ssdp->config->tx);
      ssdp->tx.state = SSD_TX_START_BIT;
      break;

    case SSD_TX_STOP:
      palSetLine(ssdp->config->tx);
    default:
      break;
    }
    osalSysUnlockFromISR();
  }
}


I'll work on these changes you asked and let you guys know.

0x3333
Posts: 57
Joined: Thu Mar 07, 2019 10:19 pm
Has thanked: 7 times
Been thanked: 6 times

Re: Doubt regarding drivers implementation

Postby 0x3333 » Fri Aug 09, 2019 4:07 pm

Here we go.

I'm almost done, just need your feedback on some points.

First, let's explain how it works.

The interface is a copy of Serial Driver, with the exception of the LLD part, which I don't have, for obvious reasons.
The driver works around a tick function, ssdTickI, that must be called by the user every port tick, which must be at least 2 times the desired bit rate, 4 being ideal. In this function, it increments a counter(ssdp->tick) and process 2 state machines, one for TX and another to RX.
Every operation on the line(TX/RX) is done in tick slots, which is calculated based on some events, start bit on RX and byte inserted in the TX queue.
The operation is really straightforward, RX will receive a start bit, wait for a bit rate multiplier + half multiplier(Start bit and half a bit), sample a bit, shift, and so on, until it will wait for a stop bit, if not received consider a framing error, otherwise, add the received byte to queue. TX will start when a byte is added to the queue, send a start bit, wait a bit rate multiplier, send bit, shift, and so on, stop bit, and restart the process.

I've tested this code in a Bluepill(STM32F103), I tested up to 115200bitrate. In high bitrates(57600), if debug is enabled(CH_DBG_STATISTICS, CH_DBG_SYSTEM_STATE_CHECK, CH_DBG_ENABLE_CHECKS, CH_DBG_ENABLE_ASSERTS, CH_DBG_TRACE_MASK, CH_DBG_ENABLE_STACK_CHECK, CH_DBG_FILL_THREADS) it could not keep up with the interruptions, they got stuck. Without debug, works great. Also, with debug enabled, I have a lot of jitter in the PAL ISR, even in slow bitrate I got some shifts(See point #3, regarding externalizing this function).

Here is some feedback I'd like to finish things up:

1 - I'm reusing status flags from Serial Driver(Different name though), but only 2 of them, is that OK to keep the same values or should start from 32 as the Serial Driver does?
I believe I should leave the same values so in the future if I implement other features, like parity, I can reuse the same value in the Serial Driver.

SERIAL Driver

Code: Select all

/**
 * @name    Serial status flags
 * @{
 */
#define SD_PARITY_ERROR         (eventflags_t)32    /**< @brief Parity.     */
#define SD_FRAMING_ERROR        (eventflags_t)64    /**< @brief Framing.    */
#define SD_OVERRUN_ERROR        (eventflags_t)128   /**< @brief Overflow.   */
#define SD_NOISE_ERROR          (eventflags_t)256   /**< @brief Line noise. */
#define SD_BREAK_DETECTED       (eventflags_t)512   /**< @brief LIN Break.  */
#define SD_QUEUE_FULL_ERROR     (eventflags_t)1024  /**< @brief Queue full. */
/** @} */

SOFT_SERIAL Driver

Code: Select all

/**
 * @name    Software Serial status flags
 * @{
 */
#define SSD_FRAMING_ERROR        (eventflags_t)64    /**< @brief Framing.    */
#define SSD_QUEUE_FULL_ERROR     (eventflags_t)1024  /**< @brief Queue full. */
/** @} */


2 - I added a function(ssdTickI) which must be called at X times the desired bitrate(I'm using 4 times), this will keep track of the driver tick and also will run the state machine for the driver. Is that OK? I used I class because of the lock needed to keep the internal state consistent.

3 - To detect the Start Bit I'm using palEnableLineEvent, but if the code is running in debug mode, I get a lot of jitter in the callback, which will affect the bit sampling. I could get rid of this by adding another function, like ssdStartBitI, so the user could use other methods to detect startBit, like fast IRQ, or something. Would this approach more likely to be accepted? Also I'd remove the dependency of PAL.

Patch attached.
Attachments
softserial.patch.zip
(6.97 KiB) Downloaded 3 times

0x3333
Posts: 57
Joined: Thu Mar 07, 2019 10:19 pm
Has thanked: 7 times
Been thanked: 6 times

Re: Doubt regarding drivers implementation

Postby 0x3333 » Fri Aug 09, 2019 8:46 pm

Just in case, this is in my git repo on GitHub:

https://github.com/0x3333/chibios_svn_m ... oft_serial

Thanks.

User avatar
Giovanni
Site Admin
Posts: 12177
Joined: Wed May 27, 2009 8:48 am
Location: Salerno, Italy
Has thanked: 570 times
Been thanked: 487 times
Contact:

Re: Doubt regarding drivers implementation

Postby Giovanni » Sun Aug 11, 2019 7:45 am

Hi,

About your points:

1) It is OK if you reuse the same values, those are two classes with a common ancestor, there would be no conflict as long names are different.

2) It is perfect, I would "standardize" an x4 clock, x2 does not allow for perfect good enough centering of samples.

3) Here I don't understand. Why you check for a front? your state machine should be able to detect a start condition: first sample to zero while in the IDLE state. You just need your clock IMHO. I would work on improving the SM for both start detection and errors handling.

Giovanni

0x3333
Posts: 57
Joined: Thu Mar 07, 2019 10:19 pm
Has thanked: 7 times
Been thanked: 6 times

Re: Doubt regarding drivers implementation

Postby 0x3333 » Mon Aug 12, 2019 12:19 pm

Hi Giovanni,

1 - OK

2 - OK, will make 4x "default" in the driver comments.

3 - In the way I conceived, the start bit was handled by an interrupt, change the state of the RX and exit. But after reading your message, I realized that it is pretty dumb. I can just read the port to check if the start bit is there or not, I'll at max, lost a 1/4 of the start bit, which I can latter recover. I'll work on these changes and let you know.

Attached a photo of this current version working(The yellow signal at the top is the ssdTickI function call, I toggle the IO).

Another photo is about the jitter I've noticed, but as I'll remove it whatsoever, probably will not be an issue anymore.
Attachments
IMG_0885.jpeg
IMG_0886.jpeg
IMG_0887.jpeg

User avatar
Giovanni
Site Admin
Posts: 12177
Joined: Wed May 27, 2009 8:48 am
Location: Salerno, Italy
Has thanked: 570 times
Been thanked: 487 times
Contact:

Re: Doubt regarding drivers implementation

Postby Giovanni » Mon Aug 12, 2019 2:09 pm

Jitter could depend on other interrupts or system critical section.

If you want to achieve maximum performance then you could drive your SM using a "fast interrupt" (priorities 0 or 1) this way it would not be affected by system critical sections. You cannot use the RTOS API from fast ISRs but you can "delegate" to another SW-triggered ISR (PENDSV) an action.

For example:
1) Your state machine works under a fast interrupt clock, no jitter, very fast IRQ processing.
2) When a frame is fully received then you set PENDSV.
3) From the PENDSV ISR (at a "normal" priority) you queue the received frame into the Input Queue.

It is quite a bit more complex however and also dependent on the Cortex-M architecture.

Giovanni

0x3333
Posts: 57
Joined: Thu Mar 07, 2019 10:19 pm
Has thanked: 7 times
Been thanked: 6 times

Re: Doubt regarding drivers implementation

Postby 0x3333 » Mon Aug 12, 2019 2:52 pm

Attached is the version with start bit detection on state machine. As usual, also in the git repo: https://github.com/0x3333/chibios_svn_m ... oft_serial

Would this be included in the Chibios as a driver? If not, I'll stop here, as it fits my needs, otherwise, I can work things out the way you believe is better for general usage, so you can include in Chibios.

Thanks for the guidance.
Attachments
softserial.patch.zip
(6.45 KiB) Downloaded 6 times


Return to “ChibiOS/HAL”

Who is online

Users browsing this forum: No registered users and 2 guests