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

Update JBL lunchbox, to utilize new luncheon library #1252

Merged
merged 1 commit into from
Mar 5, 2024

Conversation

cjswedes
Copy link
Contributor

@cjswedes cjswedes commented Mar 5, 2024

No description provided.

@cjswedes cjswedes requested review from dljsjr and sy39ju March 5, 2024 15:45
Copy link

github-actions bot commented Mar 5, 2024

Test Results

   55 files    353 suites   0s ⏱️
1 649 tests 1 649 ✅ 0 💤 0 ❌
2 896 runs  2 896 ✅ 0 💤 0 ❌

Results for commit 90c8946.

Copy link

github-actions bot commented Mar 5, 2024

Channel deleted.

Copy link

github-actions bot commented Mar 5, 2024

Minimum allowed coverage is 90%

Generated by 🐒 cobertura-action against 90c8946

Copy link
Contributor

@varzac varzac left a comment

Choose a reason for hiding this comment

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

Have we done a search on our EdgeDrivers rep to identify if there are any additional cases that we need to address?

@dljsjr
Copy link
Contributor

dljsjr commented Mar 5, 2024

Have we done a search on our EdgeDrivers rep to identify if there are any additional cases that we need to address?

I just did, and it appears that JBL/Sonos/Hue are the only drivers that were doing manual chunked transfer decode with Luncheon.

Wemo and Samsung Audio have chunked decoding codepaths, but they aren't using Luncheon/Lunchbox data types.

@cjswedes cjswedes merged commit c3ad669 into main Mar 5, 2024
11 checks passed
@cjswedes cjswedes deleted the fix/jbl-luncheon-update branch March 5, 2024 16:20
use_ssl = true
end

local port = client.base_url.port or default_port

local sock, err = client.socket_builder(client.base_url.host, port, use_ssl)
Copy link

@sy39ju sy39ju Mar 6, 2024

Choose a reason for hiding this comment

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

I think this line is required to use 4443 port.

local port = client.base_url.port or default_port

Copy link
Contributor

@dljsjr dljsjr Mar 6, 2024

Choose a reason for hiding this comment

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

@sy39ju Here are my thoughts:

1. It looks like this code was already using 443 as the default
2. 443 is the default port for HTTPS, similar to how 80 is the default port for HTTP
3. Using a port other than 443 is dependent on what port the server uses
4. Setting the port to 4443 should be done when the constructor is called, not by overriding the default port typically used by the protocol.

Disregard, I misunderstood the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants