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

Split controller OSC off from SuperDirt handshake OSC #1027

Merged
merged 5 commits into from
Aug 23, 2023

Conversation

matthewkaney
Copy link
Contributor

This is an initial step towards disentangling the controller input logic from the Stream module (as discussed in #982). Specifically, it removes the SuperDirt handshake process from the controller thread and instead handles all that per-target using the individual SuperDirt sockets. This adds support for more unusual setups (e.g. one Tidal instance running multiple SuperDirt instances at multiple addresses), but should function identically in most cases.

Fixes #793—this uses totally separate addresses for SuperDirt communication and external controllers, so it's not an issue if one is on localhost and the other isn't.

(I've done some tests on my machine, and everything seems to be working the same as in 1.9.4. I started to write some unit tests, but couldn't figure out how to write tests that didn't actually send things over UDP and got nervous that using real UDP communication could result in flaky tests.)

@matthewkaney
Copy link
Contributor Author

Pinging @Zalastax, who's expressed interest in OSC listener/stream logic, but happy to hear from anyone else with thoughts as well

Copy link
Collaborator

@Zalastax Zalastax left a comment

Choose a reason for hiding this comment

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

Looks good to me! Splendid refactoring!


resolve :: String -> String -> IO N.AddrInfo
resolve :: String -> Int -> IO N.AddrInfo
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it be a good idea to introduce a type synonyme here?
type Port = Int

then Just <$> resolve (oAddress target) (show $ fromJust $ oBusPort target)
else return Nothing
cxs <- mapM (\(target, os) -> do remote_addr <- resolve (oAddress target) (oPort target)
remote_bus_addr <- sequence (resolve (oAddress target) <$> (oBusPort target))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Smart! This uses the fact that Maybe is a Monad so sequence is the same thing as what was done here before.

@yaxu
Copy link
Member

yaxu commented Jul 26, 2023

Sorry for being a bit slow, somehow missed this, but looks great to me!

@jarmitage
Copy link

@mindofmatthew which of the 4 points you mentioned in #982 (comment) does this address? I'm guessing 1-2? Good stuff!

@matthewkaney
Copy link
Contributor Author

While trying to verify that this works correctly, I discovered that Tidal doesn't work correctly (see #1030), so I'm going to try and patch that here as well.

@jarmitage Yes, this basically cleans up the Stream module a bit. It doesn't totally separate the listener (which is my eventual plan), but it splits up two listener functions (external controllers vs. getting info from SuperDirt) that are currently combined.

@yaxu
Copy link
Member

yaxu commented Aug 23, 2023

@mindofmatthew are you happy to merge?

(I'm splitting out stream into a separate tidal-stream package on the cycseq branch.)

@matthewkaney
Copy link
Contributor Author

@yaxu Sure! I've tested and confirmed that this works. It does have a bug when SuperDirt has different control busses than 0-127 (#1030), but that's also present in 1.9, and I can address it in a separate PR.

@yaxu yaxu merged commit fcc4c5d into tidalcycles:main Aug 23, 2023
@yaxu
Copy link
Member

yaxu commented Aug 23, 2023

Thanks a lot!

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.

ControlAddr should default to 0.0.0.0 if a target isn't localhost
4 participants