sidoh / esp8266_milight_hub

Replacement for a Milight/LimitlessLED hub hosted on an ESP8266
MIT License
939 stars 220 forks source link

feat(esp32): :sparkles: add esp32 support #831

Closed Flaykz closed 2 months ago

sidoh commented 2 months ago

This looks awesome! When you're ready I should be able to run the test suite

Flaykz commented 2 months ago

@sidoh, its ready for a review. i didnt had a esp8266 to test if there is any regression :/

sidoh commented 2 months ago

That's okay! I have an automated test suite I can run against both the esp32 and esp8266.

david-brouste commented 2 months ago

Alright, I'll look at it after my workday :)

Flaykz commented 2 months ago

@sidoh, thx for the review, changes made

sidoh commented 2 months ago

Got it working! Had to add this line to platformio.ini:

[esp32]
board_build.partitions = default.csv

but I think that's because the board I used was at some point flashed with a non-default partition table. Probably still worth including this since it seems like SPIFFS maybe doesn't work with non-default partition table.

I ran the integration tests. There were some failures, but I think only 4 of them are real.

Here are the full results:

Failures:

  1) MQTT commands and state should respect the state update interval
     Failure/Error: expect(update_timestamp_gaps.length).to be >= 3

       expected: >= 3
            got:    2
     # ./spec/mqtt_spec.rb:216:in `block (3 levels) in <top (required)>'

  2) REST Server backup routes should preserve settings when creating a backup
     Failure/Error: res.value

     Net::HTTPServerException:
       400 "Bad Request"
     # ./lib/api_client.rb:119:in `block in request'
     # ./lib/api_client.rb:92:in `request'
     # ./lib/api_client.rb:144:in `upload_json'
     # ./lib/api_client.rb:152:in `block in upload_string_as_file'
     # ./lib/api_client.rb:149:in `upload_string_as_file'
     # ./spec/rest_spec.rb:366:in `block (3 levels) in <top (required)>'

  3) REST Server backup routes should override existing settings when restoring a backup
     Failure/Error: res.value

     Net::HTTPServerException:
       400 "Bad Request"
     # ./lib/api_client.rb:119:in `block in request'
     # ./lib/api_client.rb:92:in `request'
     # ./lib/api_client.rb:144:in `upload_json'
     # ./lib/api_client.rb:152:in `block in upload_string_as_file'
     # ./lib/api_client.rb:149:in `upload_string_as_file'
     # ./spec/rest_spec.rb:400:in `block (3 levels) in <top (required)>'

  4) UDP servers color and brightness commands should result in state changes
     Failure/Error:
       @client.get(topic) do |topic, message|
         ret_val = yield(topic, message)
         raise BreakListenLoopError if ret_val
       end

     Timeout::Error:
       execution expired
     # ./lib/mqtt_client.rb:90:in `block (2 levels) in on_message'
     # ./lib/mqtt_client.rb:89:in `block in on_message'

  5) UDP servers discovery should respond to v5 discovery
     Failure/Error: response, _ = @discovery_socket.recvfrom_nonblock(1024)

     IO::EAGAINWaitReadable:
       Resource temporarily unavailable - recvfrom(2) would block
     # ./spec/udp_spec.rb:127:in `block (3 levels) in <top (required)>'

  6) UDP servers discovery should respond to v6 discovery
     Failure/Error: expect(response.length).to      eq(3), "Should be a comma-separated list with three elements"
       Should be a comma-separated list with three elements
     # ./spec/udp_spec.rb:144:in `block (3 levels) in <top (required)>'

Finished in 9 minutes 16 seconds (files took 0.33943 seconds to load)
163 examples, 6 failures

Failed examples:

rspec ./spec/mqtt_spec.rb:181 # MQTT commands and state should respect the state update interval
rspec ./spec/rest_spec.rb:337 # REST Server backup routes should preserve settings when creating a backup
rspec ./spec/rest_spec.rb:383 # REST Server backup routes should override existing settings when restoring a backup
rspec ./spec/udp_spec.rb:88 # UDP servers color and brightness commands should result in state changes
rspec ./spec/udp_spec.rb:124 # UDP servers discovery should respond to v5 discovery
rspec ./spec/udp_spec.rb:135 # UDP servers discovery should respond to v6 discovery

I think we can ignore the MQTT error.

The backup route failures are real. Looks like the issue is we need to make BACKUP_FILE in Settings.h /backup.bin (instead of _backup.bin).

The first UDP failure looks like it was transient -- probably just a flaky test.

The UDP discovery failures seem real. Not the most important thing in the world -- the UDP server probably isn't used very much, but worth looking into. I'm out of time for tonight and will probably be a few days before I can look again, but suspect there's something extra we need to do for the ESP32 to allow receiving broadcast packets?

The ruby code that sends the broadcast packets is here in case it's helpful:

https://github.com/sidoh/esp8266_milight_hub/blob/master/test/remote/spec/udp_spec.rb#L119

sidoh commented 2 months ago

(I should mention that tons of tests pass -- the important stuff seems to be working just fine)

sidoh commented 2 months ago

Going to merge this into a branch in this repo so I can make the last few changes to get it over the line. tyvm for the PR <3