Go Down

Topic: Proposal for SerialComm.h ABC for Software Serial (Read 1 time) previous topic - next topic

bhagman

I'd like to propose making an abstract base class for all of the serial objects/libraries that are being created.

This will make other library developers a little happier when it comes to designing interfaces to serial devices.

Here is the proposed ABC:

Code: [Select]

#ifndef _SerialComm_h
#define _SerialComm_h

#include <stdint.h>
#include <Print.h>

/*************************************************
* Constants
*************************************************/

/*************************************************
* Abstract Base Class - SerialComm
*************************************************/
class SerialComm : public Print
{
 public:
   virtual void begin(long) = 0;
   virtual uint8_t available(void) = 0;
   virtual int read(void) = 0;
   virtual void flush(void) = 0;
   virtual void write(uint8_t) = 0;
};


#endif



This means, for example, that the only change that any existing Serial objects need to make is to inherit this class (Print comes along for the ride).

For example, mikalhart would only have to change one line in NewSoftSerial.h:

Code: [Select]

class NewSoftSerial2 : public Print


to

Code: [Select]

class NewSoftSerial2 : public SerialComm


(the same would apply to HardwareSerial.h, SoftwareSerial.h, etc)

What this allows us interface developers to do is simple polymorphism so that regardless of what a coder uses as their serial interface (be it SoftwareSerial, NewSoftSerial, the built-in HardwareSerial, or otherwise), the interface developer can use the ABC.

For example:

Here is a simple device interface:

Code: [Select]


#include <SerialComm.h>

class FooDev
{
...
 public:
   SerialComm *serial_interface;  // we could use a reference here instead to avoid dereferencing confusion at the top level
...

 FooDev::SomeMethod
 {
   ...
   if(serial_interface->available())
   {
     mychar = serial_interface->read();
   }
   ...
 }
}


All the coder does is:

Code: [Select]

#include <NewSoftSerial.h>
#include <FooDev.h>

NewSoftSerial nss(4, 5);
FooDev foo;

void setup()
{
 foo.serial_interface = &nss;  // NOTE: if we use a reference above, we could skip the "&"
}

void loop()
{
 foo.SomeMethod();  // this will interact with the serial object
}


I'd really like to see this implemented.  An ounce of interface is worth a pound of coding.

mikalhart, mellis - I'd really like to hear feedback from you guys.

b

bhagman

OK, so polymorphism isn't exactly on everyone's mind.  I get it.

What about some other base class functionality like, readnumber, readstring, etc?

Here is the new base class and extra functionality:

SerialComm.h

Code: [Select]

/* SerialComm.h

 SerialComm - Abstract Base Class

 An abstract class which can be used for just
 about any type of serialized data -
 synchronous, or asynchronous - and provides
 a base class which allows for easy polymorphism.

 Inherits Print base class to enable print and
 println functionality.

 Written by Brett Hagman

 This library is free software; you can redistribute it and/or
 modify it under the terms of the GNU Lesser General Public
 License as published by the Free Software Foundation; either
 version 2.1 of the License, or (at your option) any later version.

 This library is distributed in the hope that it will be useful,
 but WITHOUT ANY WARRANTY; without even the implied warranty of
 MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
 Lesser General Public License for more details.

 You should have received a copy of the GNU Lesser General Public
 License along with this library; if not, write to the Free Software
 Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA

Version Modified By Date     Comments
------- ----------- -------- --------
0001    B Hagman    09/06/21 Initial coding

*************************************************/

#ifndef _SerialComm_h
#define _SerialComm_h

#include <stdint.h>
#include <Print.h>

/*************************************************
* Class Definition
*************************************************/

class SerialComm : public Print
{
 public:
   // available: returns number of bytes available for reading
   // 0 if no bytes
   virtual uint8_t available(void) = 0;

   // peek: returns the byte waiting at the front of the queue
   // -1 if no bytes/error
   virtual int peek(void) = 0;

   // read: returns 1 byte if available
   // -1 if no bytes/error
   virtual int read(void) = 0;

   // write: sends a single byte through serial interface
   // NOTE: this is also needed for the Print class
   virtual void write(uint8_t) = 0;

   // flush: clears any bytes that may be in buffer
   virtual void flush(void) = 0;


   int32_t getnumber(uint8_t base);
   int16_t getstring(char *str, uint8_t size);
   int16_t getstring(char *str, uint8_t maxsize, uint8_t terminator);
   int16_t getstring(char *str, uint8_t maxsize, uint8_t terminator, bool consume_terminator);
};


#endif


SerialComm.cpp

Code: [Select]

/* SerialComm.cpp

 SerialComm - Abstract Base Class

 An abstract class which can be used for just
 about any type of serialized data -
 synchronous, or asynchronous - and provides
 a base class which allows for easy polymorphism.

 Inherits Print base class to enable print and
 println functionality.

 Written by Brett Hagman

 This library is free software; you can redistribute it and/or
 modify it under the terms of the GNU Lesser General Public
 License as published by the Free Software Foundation; either
 version 2.1 of the License, or (at your option) any later version.

 This library is distributed in the hope that it will be useful,
 but WITHOUT ANY WARRANTY; without even the implied warranty of
 MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
 Lesser General Public License for more details.

 You should have received a copy of the GNU Lesser General Public
 License along with this library; if not, write to the Free Software
 Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA

Version Modified By Date     Comments
------- ----------- -------- --------
0001    B Hagman    09/06/21 Initial coding

*************************************************/

#include "SerialComm.h"

int32_t SerialComm::getnumber(uint8_t base)
{
     uint8_t c, neg = 0;
     uint32_t val;

     val = 0;
     while(!available());
 c = peek();
 
 if(c == '-')
 {
   neg = 1;
   read();  // remove
   while(!available());
   c = peek();
 }
 
     while(((c >= 'A') && (c <= 'Z'))
         || ((c >= 'a') && (c <= 'z'))
         || ((c >= '0') && (c <= '9')))
     {
           if(c >= 'a') c -= 0x57;       // c = c - 'a' + 0x0a, c = c - ('a' - 0x0a)
           else if(c >= 'A') c -= 0x37;  // c = c - 'A' + 0x0A
           else c -= '0';
           if(c >= base) break;

           val *= base;
           val += c;
           read();                       // take the byte from the queue
           while(!available());          // wait for the next byte
           c = peek();
     }
     return neg ? -val : val;
}


int16_t SerialComm::getstring(char *str, uint8_t size)
{
 // gets a string up to size length
 
 int16_t i = 0;
 
 for(i=0; i<size; i++)
 {
   while(!available());
   str[i] = read();
 }
 str[i] = 0;
 
 return i;
}

int16_t SerialComm::getstring(char *str, uint8_t maxsize, uint8_t terminator)
{
 return getstring(str, maxsize, terminator, 1);
}

int16_t SerialComm::getstring(char *str, uint8_t maxsize, uint8_t terminator, bool consume_terminator)
{
 // gets a string up to terminator, no bigger than maxsize
 
 int16_t i = 0;
 
 for(i=0; i<maxsize; i++)
 {
   while(!available());
   if(peek() == terminator)
   {
     if(consume_terminator) read();
     break;
   }
   str[i] = read();
 }
 str[i] = 0;
 
 return i;
}


See the next post for why this is good...

bhagman

For a whopping 84 extra bytes, you can do the following:

Code: [Select]
mynumber = Serial.getnumber(HEX);

or

Code: [Select]
Serial.getstring(mystring, 40);
Serial.getstring(mystring, 40, '\n')


and MORE!

</silly_sales_pitch>

Here is a "patched" version of HardwareSerial.h and HardwareSerial.cpp.  Not much to changed really.  The class inheritance changed (to "SerialComm") and a method called peek() was added (more on that later).

(NOTE: there is another change - EmilyJane noticed that the bootloader was setting U2Xn in UCSRnA, but it was not being cleared by HardwareSerial.cpp - read more here: http://www.arduino.cc/cgi-bin/yabb2/YaBB.pl?num=1245508129 )

HardwareSerial.h

Code: [Select]

/*
 HardwareSerial.h - Hardware serial library for Wiring
 Copyright (c) 2006 Nicholas Zambetti.  All right reserved.

 This library is free software; you can redistribute it and/or
 modify it under the terms of the GNU Lesser General Public
 License as published by the Free Software Foundation; either
 version 2.1 of the License, or (at your option) any later version.

 This library is distributed in the hope that it will be useful,
 but WITHOUT ANY WARRANTY; without even the implied warranty of
 MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
 Lesser General Public License for more details.

 You should have received a copy of the GNU Lesser General Public
 License along with this library; if not, write to the Free Software
 Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
*/

#ifndef HardwareSerial_h
#define HardwareSerial_h

#include <inttypes.h>

#include "SerialComm.h"

struct ring_buffer;

class HardwareSerial : public SerialComm
{
 private:
   ring_buffer *_rx_buffer;
   volatile uint8_t *_ubrrh;
   volatile uint8_t *_ubrrl;
   volatile uint8_t *_ucsra;
   volatile uint8_t *_ucsrb;
   volatile uint8_t *_udr;
   uint8_t _rxen;
   uint8_t _txen;
   uint8_t _rxcie;
   uint8_t _udre;
   uint8_t _u2x;
 public:
   HardwareSerial(ring_buffer *rx_buffer,
     volatile uint8_t *ubrrh, volatile uint8_t *ubrrl,
     volatile uint8_t *ucsra, volatile uint8_t *ucsrb,
     volatile uint8_t *udr,
     uint8_t rxen, uint8_t txen, uint8_t rxcie, uint8_t udre, uint8_t u2x);
   void begin(long);
   uint8_t available(void);
   int read(void);
   int peek(void);
   void flush(void);
   virtual void write(uint8_t);
   using Print::write; // pull in write(str) and write(buf, size) from Print
};

extern HardwareSerial Serial;

#if defined(__AVR_ATmega1280__)
extern HardwareSerial Serial1;
extern HardwareSerial Serial2;
extern HardwareSerial Serial3;
#endif

#endif


HardwareSerial.cpp

Code: [Select]

/*
 HardwareSerial.cpp - Hardware serial library for Wiring
 Copyright (c) 2006 Nicholas Zambetti.  All right reserved.

 This library is free software; you can redistribute it and/or
 modify it under the terms of the GNU Lesser General Public
 License as published by the Free Software Foundation; either
 version 2.1 of the License, or (at your option) any later version.

 This library is distributed in the hope that it will be useful,
 but WITHOUT ANY WARRANTY; without even the implied warranty of
 MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
 Lesser General Public License for more details.

 You should have received a copy of the GNU Lesser General Public
 License along with this library; if not, write to the Free Software
 Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
 
 Modified 23 November 2006 by David A. Mellis
*/

#include <stdio.h>
#include <string.h>
#include <inttypes.h>
#include "wiring.h"
#include "wiring_private.h"

#include "HardwareSerial.h"

// Define constants and variables for buffering incoming serial data.  We're
// using a ring buffer (I think), in which rx_buffer_head is the index of the
// location to which to write the next incoming character and rx_buffer_tail
// is the index of the location from which to read.
#define RX_BUFFER_SIZE 128

struct ring_buffer {
 unsigned char buffer[RX_BUFFER_SIZE];
 int head;
 int tail;
};

ring_buffer rx_buffer = { { 0 }, 0, 0 };

#if defined(__AVR_ATmega1280__)
ring_buffer rx_buffer1 = { { 0 }, 0, 0 };
ring_buffer rx_buffer2 = { { 0 }, 0, 0 };
ring_buffer rx_buffer3 = { { 0 }, 0, 0 };
#endif

inline void store_char(unsigned char c, ring_buffer *rx_buffer)
{
     int i = (rx_buffer->head + 1) % RX_BUFFER_SIZE;

     // if we should be storing the received character into the location
     // just before the tail (meaning that the head would advance to the
     // current location of the tail), we're about to overflow the buffer
     // and so we don't write the character or advance the head.
     if (i != rx_buffer->tail) {
           rx_buffer->buffer[rx_buffer->head] = c;
           rx_buffer->head = i;
     }
}

#if defined(__AVR_ATmega1280__)

SIGNAL(SIG_USART0_RECV)
{
     unsigned char c = UDR0;
 store_char(c, &rx_buffer);
}

SIGNAL(SIG_USART1_RECV)
{
     unsigned char c = UDR1;
 store_char(c, &rx_buffer1);
}

SIGNAL(SIG_USART2_RECV)
{
     unsigned char c = UDR2;
 store_char(c, &rx_buffer2);
}

SIGNAL(SIG_USART3_RECV)
{
     unsigned char c = UDR3;
 store_char(c, &rx_buffer3);
}

#else

#if defined(__AVR_ATmega8__)
SIGNAL(SIG_UART_RECV)
#else
SIGNAL(USART_RX_vect)
#endif
{
#if defined(__AVR_ATmega8__)
     unsigned char c = UDR;
#else
     unsigned char c = UDR0;
#endif
 store_char(c, &rx_buffer);
}

#endif

// Constructors ////////////////////////////////////////////////////////////////

HardwareSerial::HardwareSerial(ring_buffer *rx_buffer,
 volatile uint8_t *ubrrh, volatile uint8_t *ubrrl,
 volatile uint8_t *ucsra, volatile uint8_t *ucsrb,
 volatile uint8_t *udr,
 uint8_t rxen, uint8_t txen, uint8_t rxcie, uint8_t udre, uint8_t u2x)
{
 _rx_buffer = rx_buffer;
 _ubrrh = ubrrh;
 _ubrrl = ubrrl;
 _ucsra = ucsra;
 _ucsrb = ucsrb;
 _udr = udr;
 _rxen = rxen;
 _txen = txen;
 _rxcie = rxcie;
 _udre = udre;
 _u2x = u2x;
}

// Public Methods //////////////////////////////////////////////////////////////

void HardwareSerial::begin(long speed)
{
     *_ubrrh = ((F_CPU / 16 + speed / 2) / speed - 1) >> 8;
     *_ubrrl = ((F_CPU / 16 + speed / 2) / speed - 1);
     cbi(*_ucsra, _u2x);
 sbi(*_ucsrb, _rxen);
 sbi(*_ucsrb, _txen);
 sbi(*_ucsrb, _rxcie);
}

uint8_t HardwareSerial::available(void)
{
     return (RX_BUFFER_SIZE + _rx_buffer->head - _rx_buffer->tail) % RX_BUFFER_SIZE;
}

int HardwareSerial::read(void)
{
     // if the head isn't ahead of the tail, we don't have any characters
     if (_rx_buffer->head == _rx_buffer->tail) {
           return -1;
     } else {
           unsigned char c = _rx_buffer->buffer[_rx_buffer->tail];
           _rx_buffer->tail = (_rx_buffer->tail + 1) % RX_BUFFER_SIZE;
           return c;
     }
}

int HardwareSerial::peek(void)
{
     if (_rx_buffer->head == _rx_buffer->tail) {
           return -1;
     } else {
           return _rx_buffer->buffer[_rx_buffer->tail];
     }
}

void HardwareSerial::flush()
{
     // don't reverse this or there may be problems if the RX interrupt
     // occurs after reading the value of rx_buffer_head but before writing
     // the value to rx_buffer_tail; the previous value of rx_buffer_head
     // may be written to rx_buffer_tail, making it appear as if the buffer
     // don't reverse this or there may be problems if the RX interrupt
     // occurs after reading the value of rx_buffer_head but before writing
     // the value to rx_buffer_tail; the previous value of rx_buffer_head
     // may be written to rx_buffer_tail, making it appear as if the buffer
     // were full, not empty.
     _rx_buffer->head = _rx_buffer->tail;
}

void HardwareSerial::write(uint8_t c)
{
     while (!((*_ucsra) & (1 << _udre)))
           ;

     *_udr = c;
}

// Preinstantiate Objects //////////////////////////////////////////////////////

#if defined(__AVR_ATmega8__)
HardwareSerial Serial(&rx_buffer, &UBRRH, &UBRRL, &UCSRA, &UCSRB, &UDR, RXEN, TXEN, RXCIE, UDRE, U2X);
#else
HardwareSerial Serial(&rx_buffer, &UBRR0H, &UBRR0L, &UCSR0A, &UCSR0B, &UDR0, RXEN0, TXEN0, RXCIE0, UDRE0, U2X0);
#endif

#if defined(__AVR_ATmega1280__)
HardwareSerial Serial1(&rx_buffer1, &UBRR1H, &UBRR1L, &UCSR1A, &UCSR1B, &UDR1, RXEN1, TXEN1, RXCIE1, UDRE1, U2X1);
HardwareSerial Serial2(&rx_buffer2, &UBRR2H, &UBRR2L, &UCSR2A, &UCSR2B, &UDR2, RXEN2, TXEN2, RXCIE2, UDRE2, U2X2);
HardwareSerial Serial3(&rx_buffer3, &UBRR3H, &UBRR3L, &UCSR3A, &UCSR3B, &UDR3, RXEN3, TXEN3, RXCIE3, UDRE3, U2X3);
#endif


b

bhagman

I can go ON and ON about this.  I also have a "patched" version of NewSoftSerial.h and NewSoftSerial.cpp.

If any one want them, let me know.

b

mellis

Can you post this to the issues list in the Arduino project on Google Code: http://code.google.com/p/arduino/issues/list?  We're trying to keep track of proposed improvements there.  A patch (universal format) is preferred, but updated files are okay too.

bhagman

Added to issues - Issue 60

I can't stress enough how important these changes are.  I also think that I've minimized the impact as much as possible, although, you could entirely remove the getstring/getnumber functions if they hinder implementing the base class.  If removed, the only addition will be the peek() function - which is, IMO, essential.

I have already written a couple of libraries that I'm waiting to release based on the addition of the peek() function at least - the getnumber/getstring functions can be implemented at a higher level if needed.

b

RoyalMisj

bhagman: THANK YOU FOR THIS!!!! You're my saviour!

cr0sh

#7
Mar 03, 2010, 05:46 pm Last Edit: Mar 03, 2010, 05:52 pm by keeper63@cox.net Reason: 1
I could really use something like this for the Pololu servo comm library I am writing; I looked at the google code issue #60 - but it is unclear whether it is in the Arduino base yet (is it in 0018?); it doesn't seem like it.

I started developing my library with SoftSerial; then I installed NewSoftSerial and implemented that - I then started thinking "what if somebody wants to use one over the other", so I added multiple constructors (function overloading) and a private flag variable to the class to tell which library was in use (with private vars holding the object pointers; the flag would allow me to tell which variable to use for the .print)...

It seems to compile fine (I have yet to do actual testing; soon...soon...) - but it seems convoluted and messy; if another serial library was to be used, you would have to add the flagging, constructor, etc for that...

I feel like I am doing something wrong, or at the least, a kludge.

With something like SerialComm.h wrapping all the other libraries, I could just use that instead of what I am doing now, and not worry about the future (provided other serial libraries used it)...

Does anybody have any suggestions? Am I approaching this wrong? I don't have my code in front of me (and it is untested, too), so I can't post it right now. Maybe I just need to leave it as-is, and in the future update it...?

:)
I will not respond to Arduino help PM's from random forum users; if you have such a question, start a new topic thread.

Udo Klein

It depends on what you want to achieve. For the suggested purpose the proposal is great. However sometimes other constraints (like memory consumption may be harder). In such cases you may even want to get less flexibility (and thus less memory consumption) like so:

Code: [Select]

#include <UT_Serial.h>

#include <avr/interrupt.h>
#include <UT_Global_types.h>

void serial__init(uint32_t baudrate) {
     cli();
     UBRR0H = ((F_CPU / 16 + baudrate / 2) / baudrate - 1) >> 8;
     UBRR0L = ((F_CPU / 16 + baudrate / 2) / baudrate - 1);

     set_bit(UCSR0B, TXEN0);
     sei();
}

void serial__print(char c) {
     while (!read_bit(UCSR0A,UDRE0)) {}

     UDR0 = c;
}


Which is a very slim but restrictred serial library.

Cheers, Udo
Check out my experiments http://blog.blinkenlight.net

Udo Klein

Oops: forgot to mention: this was not intended for maximum reuse. This is an example that was intended for one specific setup and was optimized for size.

Cheers, Udo
Check out my experiments http://blog.blinkenlight.net

Go Up