avaldebe / PMserial

Arduino library for PM sensors with serial interface
MIT License
53 stars 21 forks source link

Support Serial1 on ESP32 #7

Closed strange-v closed 4 years ago

strange-v commented 4 years ago

Hi, thank you for the great library.

I would like to use it with ESP32 HW Serial1 on custom pins (HW serial could be remapped to almost any pins on this MC) however, the library reinitializes serial interface during the init. I can fix code in the library, but it isn't a good way.

#ifdef HAS_HW_SERIAL
    static_cast<HardwareSerial*>(uart)->begin(9600, SERIAL_8N1, 16, 17);
#endif

As for me, the library shouldn't reconfigure the interface, just use it, though probably this code was added to simplify usage. Any thoughts about some kind of workaround?

avaldebe commented 4 years ago

If I remember correctly there was an problem with Serial1 on the esp32, so I flagged as unsupported**. BTW, Serial2 is on pins 16 (RX) and 17 (TX).

In any case, maybe I can do something better now. The call to begin from within the is needed in order to hide the hard/soft serial. Maybe something like the following (with all the #ifdef required) can achieve what you want.

void SerialPM::init(uint8_t rx=0, uint8_t tx=0) {
  if (hwSerial) {
    if (rx & tx) {
      static_cast<HardwareSerial *>(uart)->begin(9600, SERIAL_8N1, rx, tx);
    } else {
      static_cast<HardwareSerial *>(uart)->begin(9600, SERIAL_8N1);
    }

I know I took very long to answer, but still need think to think about it. In the meantime, I'll enable SoftwareSerial on the ESP32.

avaldebe commented 4 years ago

I won't enable SoftwareSerial on the ESP32, so could use the SerialPM(sensor, rx, tx) constructor as follows:

#ifdef HAS_SW_SERIAL
  SerialPM(PMS sensor, uint8_t rx, uint8_t tx) : pms(sensor)
  {
    SoftwareSerial serial(rx, tx);
    uart = &serial;
    hwSerial = false;
  }
#elif defined(ESP32)
  SerialPM(PMS sensor, uint8_t rx, uint8_t tx) : pms(sensor), rx(rx), tx(tx)
  {
    uart = &Serial1;
    hwSerial = true;
  }
#endif
strange-v commented 4 years ago

It's okay as a workaround. However, do you see any disadvantages of using Stream * stream to work with both software and hardware serial? I still think that the best option is to use a preconfigured interface by the user, so you don't have care about the type (software/hardware) and pins. Of course, it is up to you, but I use this approach here without any issues.

avaldebe commented 4 years ago

I see your point. However, it is "standard practice" to hide the wire.begin call on I2C sensor libraries, so I see no problem to do the same here.

On a library I prefer the "tell me what you want" approach over the "tell me how to do it". The SerialPM(sensor, serial) constructor set the sensor on the pins of a predefined serial port, the SerialPM(sensor, rx, tx) constructor sets the sensors on whatever pins you want. The fact that it is a HardwareSerial or a SoftwareSerial behind should be handled by the library not the user.

strange-v commented 4 years ago

it is "standard practice" to hide the wire.begin call on I2C sensor libraries

As with UART, i2c pins can be remapped... But, in general, I understand your point.