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

Adding support multiple GPIO for DS18x20 sensors. Aliases of DS18x20 optimization. #16833

Merged
merged 14 commits into from
Oct 18, 2022

Conversation

md5sum-as
Copy link
Contributor

Description:

Adding support multiple GPIO for DS18x20 sensors.
To enable this function, use the DS18x20_MULTI_GPIOs flag in my_user_config.h or user_config_override.h
Discussion: #16811
Optimization a code in xsns_05_ds18x20.ino and xsns_05_esp32_ds18x20.ino in function Ds18x20Name()

Checklist:

  • The pull request is done against the latest development branch
  • Only relevant files were touched
  • Only one feature/fix was added per PR and the code change compiles without warnings
  • The code change is tested and works with Tasmota core ESP8266 V.2.7.4.9
  • The code change is tested and works with Tasmota core ESP32 V.2.0.5
  • I accept the CLA.

NOTE: The code change must pass CI tests. Your PR cannot be merged unless tests pass

Copy link
Contributor

@barbudor barbudor left a comment

Choose a reason for hiding this comment

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

I've put some comments because I think this could be optimized and impact on code size reduced

Comment on lines -451 to +517
snprintf_P(DS18X20Data.name, sizeof(DS18X20Data.name), PSTR("DS18Alias%c%d"), IndexSeparator(), ds18x20_sensor[sensor].alias);
} else {
#endif
snprintf_P(DS18X20Data.name, sizeof(DS18X20Data.name), PSTR("%s%c%d"), DS18X20Data.name, IndexSeparator(), sensor +1);
#ifdef DS18x20_USE_ID_ALIAS
snprintf_P(DS18X20Data.name, sizeof(DS18X20Data.name), PSTR("DS18Sens"));
print_ind = ds18x20_sensor[sensor].alias;
Copy link
Contributor

Choose a reason for hiding this comment

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

Changing from "DS18Alias" to "DS18Sens" is now a breaking change
I know you introduced that recently so probably not much is use yet but just raising the point

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I recently added this functionality and it hasn't been released yet. DS18Sens is a more correct name. I will try to avoid such edits. Also fixed aliases output to the accepted style.

Comment on lines +73 to +81
#ifdef DS18x20_MULTI_GPIOs
struct {
int8_t pin = 0; // Shelly GPIO3 input only
int8_t pin_out = 0; // Shelly GPIO00 output only
bool dual_mode = false; // Single pin mode
} ds18x20_gpios[MAX_DSB];
uint8_t ds18x20_ngpio = 0; // Count of GPIO found
#endif

Copy link
Contributor

Choose a reason for hiding this comment

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

Why introducing a new data structure instead of modulating the size of the existing DS18X20Data between [1] and [MAX_DSB]
That should simplify the rest of the code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The new structure uses only 12 bytes of RAM. DS18X20Data uses 22 bytes and increasing to 4 copies will add another 66 bytes.
And will also have to pass GPIO numbers to all low-level functions: OneWire1ReadBit(void), OneWire2ReadBit(void), OneWireWriteBit(uint8_t v), OneWireReset(void). Such changes can lead to a significant increase in the final code.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's what I was wondering : passing some parameters to function vs copying

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made a test module, replaced copying with transferring the index of the gpio array.
With copying use 687300 bytes of flash.
With passing the parameter to the low-level functions 687364 bytes of flash.
Loss of 64 bytes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for trying

@arendst arendst added the on hold by dev team Result - Feature request put on hold by member of development team label Oct 16, 2022
@arendst
Copy link
Owner

arendst commented Oct 16, 2022

Set to on hold to merge after next release.

@arendst arendst self-assigned this Oct 18, 2022
@arendst arendst merged commit df24aef into arendst:development Oct 18, 2022
arendst added a commit that referenced this pull request Oct 18, 2022
Add DS18x20 support on up to four GPIOs by md5sum-as (#16833)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
on hold by dev team Result - Feature request put on hold by member of development team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants