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

Added the SerialConfig command, providing the ability to set the data bits/parity/stop bits parameters for the hardware UART #7108

Closed
wants to merge 4 commits into from

Conversation

teixeluis
Copy link
Contributor

@teixeluis teixeluis commented Dec 4, 2019

Description:

In order to facilitate configuration based integration with peripherals/secondary MCUs or other devices which require less usual serial port settings, I have implemented the necessary changes (contained in this PR) to the code base, allowing the user to modify these parameters anytime, in a similar fashion to the Baudrate command. The new command is called SerialConfig, and like other commands can be called without parameters, to retrieve the current setting:

21:26:01 CMD: SerialConfig
21:26:01 RSL: stat/sonoff/RESULT = {"SerialConfig":0}

As you can see in more detail below, 0 corresponds to the 7N1 setting. The command also accepts 1, 2 and 3, for the other possible settings. Changing the current setting, is a matter of passing this value as a parameter. For example to configure as 8N1:

21:26:01 CMD: SerialConfig 2
21:26:01 RSL: stat/sonoff/RESULT = {"SerialConfig":2}

Given the limitations of the device, the command only covers the hardware serial port. Eventually some trickery may allow the same for software serial bridge, but I have decided not to delve into that in this phase, given the complexity and number of potential changes to the codebase that it would require.

In order to avoid disrupting too much the current settings structure, and also prevent having to add an extra block of flash memory (the current settings struct is already at the limit of the 4096 bytes erase block size) because of just a single field, I have decided to use the location where the baudrate field was, and in the same 16 bits, encode the same baudrate bits (only 14 bits are required for covering the multiples of 300 until the maximum theoretical baud rate the ESP8266 supports, which is 4608000 bps):

...
  uint16_t      display_height;            // 776
  uint16_t      serial_config;             // 778 - 14 MSB's define the baud rate; 2 LSB's define the serial config (e.g. 8N1). Maps to the SerialCfg struct.
  uint16_t      sbaudrate;                 // 77A
...

and also use 2 bits for encoding a selection of the serial port modes which in principle are more likely to be used. The selected settings are:

  0 - 7N1 (7 data bits / no parity / 1 stop bit)
  1 - 7E1 (7 data bits / even parity / 1 stop bit)
  2 - 8N1 (8 data bits / no parity / 1 stop bit)
  3 - 8E1 (8 data bits / even parity / 1 stop bit)

This change doesn't break the compatibility with backups from previous versions, however the Baudrate and SerialConfig will have to be reconfigured after a restore.

The motivation to write this feature came as a consequence of learning more about the ZMAi-90 DIN rail meter/switch device (which is shipped with a firmware that uses the Tuya cloud infrastructure), while analysing and doing some level of reverse engineering to it. The metering MCU of this device uses 9600 bps 8E1 settings, which Tasmota couldn't provide out of the box. More details can be viewed in the blog post below:

[DIN-rail ZMAi-90 meter/switch reverse engineering]
(https://www.creationfactory.co/2019/11/attempting-to-reverse-engineer-home.html)

In the future I plan to dedicate effort to write a specific device driver in Tasmota for this device, but for the time being, this feature opens the door for an immediate configuration-based integration, which is particularly interesting for the community.

Checklist:

  • The pull request is done against the latest dev branch
  • Only relevant files were touched
  • Only one feature/fix was added per PR.
  • The code change is tested and works on core 2.6.1
  • The code change pass travis tests. Your PR cannot be merged unless tests pass
  • I accept the CLA.

Glad to be able to contribute to this great project.

Cheers,

Luis Teixeira

Copy link
Contributor

@ascillato ascillato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add the translation key for all languages in order to pass travis tests and to not break the automated process for precompiled bins.

If travis tests are not passed, this PR cannot be merged.

Thanks

@teixeluis
Copy link
Contributor Author

Please add the translation key for all languages in order to pass travis tests and to not break the automated process for precompiled bins.

If travis tests are not passed, this PR cannot be merged.

Thanks

@ascillato are Google translate translations enough for the time being? :)

Cheers

@ascillato
Copy link
Contributor

No need to actually translate them. Just put them in English and then, other users will update them.

@arendst
Copy link
Owner

arendst commented Dec 5, 2019

Thx for your input. You thought it out very well. I have just a few remarks ;-)

  • I would suggest to use option 0 as the current default 8N1. This solves backward compatibility as the top bits of the baudrate uint16_t would be zero now anyway. But releated to the next bullet you do not need to change this...
  • I'm struggling with the possible supported SerialConfigs and SerialModes by the core:
enum SerialConfig {
    SERIAL_5N1 = UART_5N1,
    SERIAL_6N1 = UART_6N1,
    SERIAL_7N1 = UART_7N1,
    SERIAL_8N1 = UART_8N1,
    SERIAL_5N2 = UART_5N2,
    SERIAL_6N2 = UART_6N2,
    SERIAL_7N2 = UART_7N2,
    SERIAL_8N2 = UART_8N2,
    SERIAL_5E1 = UART_5E1,
    SERIAL_6E1 = UART_6E1,
    SERIAL_7E1 = UART_7E1,
    SERIAL_8E1 = UART_8E1,
    SERIAL_5E2 = UART_5E2,
    SERIAL_6E2 = UART_6E2,
    SERIAL_7E2 = UART_7E2,
    SERIAL_8E2 = UART_8E2,
    SERIAL_5O1 = UART_5O1,
    SERIAL_6O1 = UART_6O1,
    SERIAL_7O1 = UART_7O1,
    SERIAL_8O1 = UART_8O1,
    SERIAL_5O2 = UART_5O2,
    SERIAL_6O2 = UART_6O2,
    SERIAL_7O2 = UART_7O2,
    SERIAL_8O2 = UART_8O2,
};

enum SerialMode {
    SERIAL_FULL = UART_FULL,
    SERIAL_RX_ONLY = UART_RX_ONLY,
    SERIAL_TX_ONLY = UART_TX_ONLY
};

I know chances are small someone wants to use SERIAL_5N1 but i've been there before. As soon as you think supporting the mainstream must do someone pops up and needs this Serial_5N1.

In that case we probably have to support that too. So perhaps we leave the baudrate alone and add another uint8_t for the 24 possible SerialConfigs. Let's forget the SerialMode for now.

Using the info from the core file HardwareSerial.h we make an array of uint8_t with size 24 providing all 24 SerialConfig options. Using that array we set the default to index 3 in Settings.ino and increase the version number to make sure the default serialconfig stays at 8N1.
Using the enum of the core we set the default to

@arendst arendst self-assigned this Dec 6, 2019
@arendst
Copy link
Owner

arendst commented Dec 6, 2019

@teixeluis I'm currently rewriting this. So no action on your part needed.

@arendst arendst added the Awaiting feedback from Project Owner Awaiting feedback / input from Project Owner label Dec 6, 2019
@teixeluis
Copy link
Contributor Author

Hi @arendst,

Thank you for your feedback :) Sorry yesterday I had a full day couldn't look into your inputs.

Yes it makes perfect sense to support the full range of serial settings. What got me into the intermediate approach was the problem of increasing the size of the settings.

In the approach you described, what would the array be for exactly? Pointing to its 24 entries leaves us with the same problem of needing more bits in the struct.

I first thought of assigning less bits to the baudrate (i.e. 11 bits), and use the other 5 bits to cover the full 24 config modes, but this would only take us to a maximum of 614100 bps, assuming the same multiple of 300.

You probably have other ideas for tackling this, but at a first glance, increasing the settings struct size got me into the problem of due to byte alignment, the structure growing beyond the 4096 bytes.

The next natural step to accomodate all this would be to have it use another 4096 bytes of flash, with the backward-compatibility issues this may carry (eventually not too much of a problem to address, if the existing properties are kept in the first block).

Thanks and looking forward for your changes.

Cheers,

Luis Teixeira

@arendst
Copy link
Owner

arendst commented Dec 6, 2019

Thx for your concerns regarding flash usage.

Increasing the size is not needed as there are managed free spaces within the 4096 bytes flash. I already claimed a byte for the serial config. To be released soon.

@teixeluis teixeluis closed this Dec 6, 2019
arendst added a commit that referenced this pull request Dec 6, 2019
Add command ``SerialConfig 0..23`` or ``SerialConfig 8N1`` to select Serial Config (#7108)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting feedback from Project Owner Awaiting feedback / input from Project Owner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants