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

WIP: Add non-blocking mode #34

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

Simon-L
Copy link

@Simon-L Simon-L commented Sep 30, 2023

Please do not merge as is :)

Related to #33 and #32
Attempted to add necessary methods for non-blocking, as far as I can tell this is working.

Basically it sets a timeout on receive so poll() just returns and other things can happen inbetween polling for new messages, what do you think? I haven't checked using other plugins with these changes.

Note that the new example has a temporary line to run from the source repo and bypas globally installed losc

@@ -0,0 +1,40 @@
-- REMOVE NEXT LINE BEFORE MERGING
package.path = ';./src/?/init.lua;./src/?.lua;/usr/local/share/lua/5.1/?.lua'
Copy link

Choose a reason for hiding this comment

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

Looks like you intended on removing this before creating the PR?

Copy link
Author

Choose a reason for hiding this comment

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

I will remove it in an upcoming commit, like the comment and the PR says this is just a quick draft I put together for @davidgranstrom and you to review, thanks
But I guess eveyone installs with luarocks makeand this was not even needed in the first place!

@@ -158,6 +158,13 @@ function losc:open(...)
return pcall(self.plugin.open, self.plugin, ...)
end

function losc:poll(...)
Copy link

Choose a reason for hiding this comment

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

you beat me to the PR!
I would add some documentation here:

--- Polls an OSC server.
-- Used by non-blocking plugins.
-- @param[opt] ... Plugin specific arguments.
-- @return bool representing if the plugin dispatched a message
-- @usage losc:poll()

Copy link
Author

Choose a reason for hiding this comment

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

Thanks I'll add it!
I should maybe link to the PIL page that has an example of this as well: https://www.lua.org/pil/9.4.html
It's about coroutines but the purpose of the :timeout() method is clearly shown

@davidgranstrom
Copy link
Owner

Hi @Simon-L and thanks for this PR! I think poll would be a great addition to the API. I have a little different thought about the API though, how about if poll would return the OSC packet and it would be up to the user to dispatch it? This would make the API a bit more flexible IMHO in regards to different threading scenarios (e.g. sending the packet to a different thread for dispatch)

The signature could look like this:

--- Poll raw OSC packets.
-- @param[opt] ... Plugin specific arguments.
-- @return status, osc packet bytes or nil/error
-- @usage
-- -- nil is a valid return value so always check status and packet
-- -- before accessing the data.
-- local status, packet = losc:poll()
-- if status and packet then
--   -- do something with packet
-- end
function losc:poll(...)
  if not self.plugin then
    error('"poll" must be implemented using a plugin.')
  end
  return pcall(self.plugin.poll, self.plugin, ...)
end

This would be the implementation (for luasocket)

-- non-blocking
function M:open(host, port, timeout)
  host = host or self.options.recvAddr
  host = socket.dns.toip(host)
  port = port or self.options.recvPort
  self.handle:setsockname(host, port)
  self.handle:settimeout(timeout or 0.0)
end

function M:poll()
  return (self.handle:receive())
end

And the main loop of your example would look like this:

local i = 0
while true do
  print("Loop iteration " .. i)
  require'socket'.select(nil, nil, 0.25) -- equivalent for sleep() to simulate other tasks
  local status, data = osc:poll()
  if status and data then
    local ok, err = pcall(Pattern.dispatch, data, udp)
    if not ok then
      print(err)
    end
  end
  i = i + 1
end

We could also add losc:dispatch(data) to the API as a convenience function for the above.

Let me know what you think!

@halee88
Copy link

halee88 commented Oct 8, 2023

@davidgranstrom I like the idea of having the dispatch be separate from the poll()/plugins. I think this could be taken further and fully separate the dispatch/handler system into its own class (dispatcher as a sibling of pattern, packet, serializer) and could be completely optional for users of your library (eg. an app that just forwards raw packets to another endpoint wouldn't necessarily care about osc addresses) That being said I don't mind poll() executing the dispatching directly. It follows the original spirit of your library and it works great in my usage case!

@Simon-L
Copy link
Author

Simon-L commented Oct 9, 2023

Hey there thanks for the thoughts. I absolutely agree about not automatically dispatching although semantically speaking a non-blocking mode doesn't imply dispatching any differently than the blocking mode, but I get the idea!

This also brings to the table the idea of being able to (re)route osc messages.

@halee88 As far as I can tell from the code and @davidgranstrom's examples above, Pattern.dispatch is already very much decoupled from the rest of the library, which is very handy for what you suggest.

@halee88
Copy link

halee88 commented Oct 9, 2023

@Simon-L ahh good catch! I didn't see that pattern had encapsulated everything needed

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.

3 participants