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

Fix HTTPRequestComponent::get_string return value #2987

Merged
merged 4 commits into from
Jan 3, 2022

Conversation

martgras
Copy link
Contributor

@martgras martgras commented Jan 3, 2022

What does this implement/fix?

the static variable initialization happens only once therefore http_request.get_string() returns stale values.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Other

Related issue or feature (if applicable): fixes esphome/issues#2903

Pull request in esphome-docs with documentation (if applicable): esphome/esphome-docs#

Test Environment

  • ESP32
  • ESP32 IDF
  • ESP8266

Example entry for config.yaml:

esphome:
  name: testm5       
esp32:
  board: pico32
  framework:
    type: arduino
logger:
  level: VERBOSE
wifi:
  networks:  
      ssid: !secret wifi_sid
      password: !secret wifi_password
http_request:
  useragent: esphome/device
  id: httpget
  timeout: 20s
sensor:
  - platform: uptime
    name: Uptime Sensor
    id: uptime_sensor
    update_interval: 20s
    on_value:
      then:
        - http_request.get:
            url: http://worldtimeapi.org/api/timezone/Europe/Berlin
            on_response:
              then:
                - logger.log:
                    format: "Got %s"
                    args: ['id(httpget).get_string()']

Checklist:

  • The code change is tested and works locally.
  • Tests have been added to verify that the new code works (under tests/ folder).

If user exposed functionality or configuration variables are added/changed:

@oxan oxan changed the title fix get_string return value Fix HTTPRequestComponent::get_string return value Jan 3, 2022
@oxan oxan merged commit dce3713 into esphome:dev Jan 3, 2022
@roitagar
Copy link
Contributor

roitagar commented Jan 3, 2022

Regarding trying to avoid copying - will it work if you make the local static variable of type String rather than std::string?
It'll allow use of a Move Constructor..

@oxan
Copy link
Member

oxan commented Jan 3, 2022

On ESP8266 getClient() returns a String& to a member variable, and I don't think it likes if we move from that object.

@roitagar
Copy link
Contributor

roitagar commented Jan 3, 2022

I think the compiler handles it automatically, so ESP8266 will use a copy-constructor and ESP32 will use a move-constructor.

EDIT: Obviously I mean copy-assignment vs. move-assignment and not constructor - as the static object is already constructed at that point..

@oxan
Copy link
Member

oxan commented Jan 3, 2022

Hmm, I think you're right and that'd work. Happy to see (another) PR change it into that :)

@roitagar
Copy link
Contributor

roitagar commented Jan 3, 2022

@oxan

Happy to see (another) PR change it into that :)

#2988 😃

@github-actions github-actions bot locked and limited conversation to collaborators Jan 5, 2022
@martgras martgras deleted the http-fix branch January 10, 2022 08:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

http_request.get: get_string() always returns the same (first) value
3 participants