PDA

View Full Version : CLI versus RPC



Jacob Potter
2005-08-27, 11:06
Hi all -

I've been working quite a bit lately on my RPC SlimServer interface,
and I've noticed that a lot of the code in Command.pm is focused
towards the CLI interface in particular. Command::execute() returns a
flat array of strings (sometimes just values, sometimes name:value
pairs), so it is possible to use it for other things, but requires
some unnecessary re-parsing.

I think the right thing to do is split out execute() in to two
functions, execute() for actual "commands" and query() for data
retrieval. Both of these would still be part of Command.pm, and common
to the CLI, RPC, stdio, etc. interfaces. Rather than returning data as
CLI formatted strings, query() would give back whatever value is
appropriate for the function called - be it a scalar, hash, array, or
some combination. All the CLI-specific encoding code would be in
CLI.pm, much like RPC.pm's mini function handlers are separate from
the XML- and JSON-RPC-specific code.

I wish I'd posted this before Fred's patch this morning (sorry!)...
that does the command / query separation, but doesn't make them any
more general-purpose.

Make sense? Any comments?

- Jacob

Fred
2005-08-29, 10:20
Jason,

I started with what was there initially. Hence queries in execute(). With time, I grew more confident about making architectural changes, hence my latest patch, really motivated by practicalities and the same basic consideration: an array is really a bit "simple" as a systematic return value.

I find the CLI is a very elegant solution, sharing the external protocol with the internal one, but with the proliferation of external access protocols (RPC, xPL, etc) and the server complexity growing, I do not think this is much viable much longer.

I would see a common command/query layer accessed by all protocols, including the web interface (it is mostly so today for commands, but not for queries). After all the CLI database browse methods only try to replicate the web interface.

Now if the common query layer returns something different for every query, then you basically have to individually code the "protocol handler" for every query. The same applies for the commands as the same argument can be made: why use an array for every command?

Likewise, we could discuss why RPC is a plugin when CLI and xPL are not?

Given there isn't generally a large excitement on this list for things CLI, I decided to go ahead and patch enough to solve my own little needs, but I am entirely ready to reconsider my approach should another one gain more momentum in the community.

Fred

Jacob Potter
2005-08-29, 11:22
On 8/29/05, fred @ thomascorner. com
<fredthomascorner.com.1ujdfo (AT) no-mx (DOT) forums.slimdevices.com> wrote:
> Jason,

Jacob. :) ("JSON", the data format, isn't my invention.)

> I would see a common command/query layer accessed by all protocols,
> including the web interface (it is mostly so today for commands, but
> not for queries). After all the CLI database browse methods only try to
> replicate the web interface.

Exactly what I'd like to have as well. We have it now for commands,
but not queries...

> Now if the common query layer returns something different for every
> query, then you basically have to individually code the "protocol
> handler" for every query. The same applies for the commands as the same
> argument can be made: why use an array for every command?

It seems to me that the basic Perl array/hash constructs could be
mapped fairly directly to the CLI. Consider the Perl:
{
"listitems" => [
{
"foo" => "bar1",
"baz" => "quux1"
}, {
"foo" => "bar2",
"baz" => "quux2""
}
]
}

>From what I can tell, translating that to CLI-array notation would
look something like:
"listitems index:1" "foo:bar1" "baz:quux1" "listitems index:2"
"foo:bar2" "baz:quux2"

It's not quite as straightforward to go from Perl to the CLI as it is
to JSON or XML-RPC, but it can be done. The RPC plugin implements both
XML-RPC and JSON-RPC, but the "commandlets" (doCommand, getPlaylist,
getStrings, etc) are only implemented once, and return a standard Perl
array or hash.

> Likewise, we could discuss why RPC is a plugin when CLI and xPL are
> not?

I implemented it originally as a plugin (to make it easier to write,
IIRC) and asked on the developer lists; Dan Sully suggested I check it
in as such. It would be nice to have it part of the core server itself
in the future.

> Given there isn't generally a large excitement on this list for things
> CLI, I decided to go ahead and patch enough to solve my own little
> needs, but I am entirely ready to reconsider my approach should another
> one gain more momentum in the community.

It's a great system; I just think the internals could be a bit more
versatile. :)

- Jacob

robinbowes
2005-08-29, 14:08
Guys,

Dan suggested I join in this thread (mainly to stop me pestering him with blue sky architectural stuff, I'm sure :) )

I'm trying to re-think how the whole slimserver code is architected to make it more modular and therefore easier to maintain and develop.

Rather than explain it all up front, can you review the diagram here [1] and let me have your comments then we can discuss?

[1] http://robinbowes.com/filemgmt/visit.php?lid=9

R.

Jacob Potter
2005-08-29, 16:01
On 8/29/05, robinbowes <robinbowes.1ujnuq (AT) no-mx (DOT) forums.slimdevices.com> wrote:
> Rather than explain it all up front, can you review the diagram here
> [1] and let me have your comments then we can discuss?

Looks pretty nice to me, although I don't know that there should be a
connection between "Output Controller" and "Web Controller". I was
thinking of "Output" as handling the player menus and such, while
things like current playlist and player state would be handled by the
main control code.

You're right, it doesn't really make as much sense when written out...
what'd you use to make the pretty graphics? :)

- Jacob

Robin Bowes
2005-08-29, 23:15
Jacob Potter said the following on 30/08/2005 00:01:
> On 8/29/05, robinbowes <robinbowes.1ujnuq (AT) no-mx (DOT) forums.slimdevices.com> wrote:
>
>>Rather than explain it all up front, can you review the diagram here
>>[1] and let me have your comments then we can discuss?
>
>
> Looks pretty nice to me, although I don't know that there should be a
> connection between "Output Controller" and "Web Controller". I was
> thinking of "Output" as handling the player menus and such, while
> things like current playlist and player state would be handled by the
> main control code.

Well, perhaps "Output Controller" is the wrong term. I envisage one
piece of code pulling together the metadata and other real-time
information on what's playing now and the web controller / VFD
controller simply formatting it and physically driving the display.

This is only a first draft (well, it's about the third actually, but
first I've "released"!).

There are quite a few links missing around where the metadata
information comes from, etc.

Also, I've not touched on SqueezeNetwork or integration with
iTunes/MusicMagic/etc.

I'd like to see each of the processes as much of a "black box" as
possible. Each process should do one job and do it well with a clearly
defined set of inputs and outputs. This will make development and
testing much easier.

It will also make it a lot easier to split the Slimserver code across
threads/processes thus addressing performance issues. For example, if
the streaming is in a different process to the scanning, playback will
not be affected by the scanning process.

> You're right, it doesn't really make as much sense when written out...
> what'd you use to make the pretty graphics? :)

Visio 2003.

R.

--
http://robinbowes.com

If a man speaks in a forest,
and his wife's not there,
is he still wrong?

Grotus
2005-08-30, 07:46
robinbowes wrote:
> Guys,
>
> Dan suggested I join in this thread (mainly to stop me pestering him
> with blue sky architectural stuff, I'm sure :) )
>
> I'm trying to re-think how the whole slimserver code is architected to
> make it more modular and therefore easier to maintain and develop.
>
> Rather than explain it all up front, can you review the diagram here
> [1] and let me have your comments then we can discuss?
>
> [1] http://robinbowes.com/filemgmt/visit.php?lid=9
>
> R.
>
>

Splitting the IR Controller from the VFD Controller probably won't work
as both of these need to communicate on the same socket connection.
There would probably need to be a single process for bidirectional
communication with the clients (or one process/thread per client). That
process would then communicate with both the Main Controller and the
Output Controller.

Robin Bowes
2005-08-30, 08:01
Robert Moser wrote:
> robinbowes wrote:
>
>> Guys,
>>
>> Dan suggested I join in this thread (mainly to stop me pestering him
>> with blue sky architectural stuff, I'm sure :) )
>>
>> I'm trying to re-think how the whole slimserver code is architected to
>> make it more modular and therefore easier to maintain and develop.
>>
>> Rather than explain it all up front, can you review the diagram here
>> [1] and let me have your comments then we can discuss?
>>
>> [1] http://robinbowes.com/filemgmt/visit.php?lid=9
>>
>> R.
>>
>>
>
> Splitting the IR Controller from the VFD Controller probably won't work
> as both of these need to communicate on the same socket connection.
> There would probably need to be a single process for bidirectional
> communication with the clients (or one process/thread per client). That
> process would then communicate with both the Main Controller and the
> Output Controller.

Robert,

That's the sort of feedback I'm looking for.

However, this is a logical diagram. The physical implementation may
involve a single process containing both controllers.

I'll need to take issues like this into consideration when I get down to
the physical diagram.

Incidentally, why do the IR controller and VFD controller need to use
the same socket? Is this a feature of the SB firmware?

R.
--
http://robinbowes.com

If a man speaks in a forest,
and his wife's not there,
is he still wrong?

Grotus
2005-08-30, 11:34
Robin Bowes wrote:
> Robert,
>
> That's the sort of feedback I'm looking for.
>
> However, this is a logical diagram. The physical implementation may
> involve a single process containing both controllers.
>
> I'll need to take issues like this into consideration when I get down to
> the physical diagram.
>
> Incidentally, why do the IR controller and VFD controller need to use
> the same socket? Is this a feature of the SB firmware?
>
> R.

The SB uses one TCP connection for commands and display and another for
streaming audio. That is indeed part of the firmware.

Having a Client Controller object in the diagram with an input of Remote
and an output of Display which has connections to the Main Controller
and Output controller makes sense logically too. Done in that way it
mirrors the Web Controller object, which leads to a generalized concept
of a Controller which acts as an intermediary between an I/O mechanism
and the SlimServer Main and Output Controllers. So, then you can extend
your logical diagram to include an RPC Controller, a CLI Controller, etc.

Fred
2005-10-30, 18:05
Guys,

I gave all of this some thinking and now is the time to act for 6.5.

We have at least 5 "Client Controllers" today: Client/Player, web, CLI, xPL and RPC. Not sure if MoodLogic qualifies... Ideally, "Client Controllers" should be possible as plug-ins.

We need something generic these clients can send commands and execute queries to. This generic thing needs to be extendable by plug-ins. This generic thing shall provide hooks of some sort so that controllers can make their clients aware of changes (performed by the Main Controller or any other Client Controller). There shall be some form of error reporting. Future threading shall be designed in the design.

I am looking for practical implementation ideas...

Fred

dean
2005-10-31, 00:12
There are three different levels that you touch on:

1. User interface (the player display/remote, the web HTML)
2. Protocols (HTTP, XPL, CLI, RPC)
3. Underlying command structure.

#1 and #2 probably shouldn't change too much, at least at first, but
there's lots of low hanging fruit to make #3 better.

We have one bottleneck in the server which is the
Slim::Control::Command module, specifically the
Slim::Control::Command::execute() function.

That's the place to start looking and thinking about this.

The first thing I'd do to that function is make it tiny. Each of
those "elsif ($p0 eq "commandname")" clauses should be moved to
separate functions which are looked up in a hash by the new execute
(). Then adding functions is as easy as adding entries to that hash.

And yes, you can blame me for that abomination.

-dean

On Oct 30, 2005, at 5:05 PM, fred (AT) thomascorner (DOT) com wrote:

>
> Guys,
>
> I gave all of this some thinking and now is the time to act for 6.5.
>
> We have at least 5 "Client Controllers" today: Client/Player, web,
> CLI,
> xPL and RPC. Not sure if MoodLogic qualifies... Ideally, "Client
> Controllers" should be possible as plug-ins.
>
> We need something generic these clients can send commands and execute
> queries to. This generic thing needs to be extendable by plug-ins.
> This
> generic thing shall provide hooks of some sort so that controllers can
> make their clients aware of changes (performed by the Main Controller
> or any other Client Controller). There shall be some form of error
> reporting. Future threading shall be designed in the design.
>
> I am looking for practical implementation ideas...
>
> Fred
>
>
> --
> fred (AT) thomascorner (DOT) com
>

Fred
2005-10-31, 16:00
Dean,

For execute(), yes of course. Queries and functions must be separated as well, because f.e. queries are lower priority and have no need to be "listened" to. Plug-ins need the capability to add stuff to the hashes, so that they can make their commands and queries available to all (and in particular the CLI, to support Internet Radio from it).

A hash of some sort looks like the ideal standard command and/or response format. In the CLI I will have to maintain the order of parameters and responses, but I can't see a way out of it except with ordered hashes. RPC/web/player probably care less about that.

Now if we have separate functions for each command, then is execute still necessary at all? Probably yes to provide some sort of command framework, like callbacks and the like.

Now 2 questions:

- Do we want to unify CLI/xPL and RPC as either plug ins or base code? Dean do you have any view on that? I was thinking having them as plugin would enable people not using them to disable them, therefore saving resources... xPL even has its own disabling mechanism.

- Do we want to somehow go through idle in execute? (i.e. have all commands / queries be handled asynchronously). That would enable throttling or otherwise making sure players play. This should map more or less elegantly to some threading mechanism or POE or whatever. Some in house very simple event mechanism should do it.

Comments welcomed

Fred

Dan Sully
2005-10-31, 16:11
* fred (AT) thomascorner (DOT) com shaped the electrons to say...

>For execute(), yes of course. Queries and functions must be separated
>as well, because f.e. queries are lower priority and have no need to be
>"listened" to. Plug-ins need the capability to add stuff to the hashes,
>so that they can make their commands and queries available to all (and
>in particular the CLI, to support Internet Radio from it).

Yes - there should be an API for adding queries. The caller should have no
idea what the internal jump-table mechanism is.

>Now 2 questions:
>
>- Do we want to unify CLI/xPL and RPC as either plug ins or base code?
>Dean do you have any view on that? I was thinking having them as plugin
>would enable people not using them to disable them, therefore saving
>resources... xPL even has its own disabling mechanism.

I'm inclined to say plugins. The vast majority of users will never touch the
CLI or xPL. RPC is already a plugin.

>- Do we want to somehow go through idle in execute? (i.e. have all
>commands / queries be handled asynchronously). That would enable
>throttling or otherwise making sure players play. This should map more
>or less elegantly to some threading mechanism or POE or whatever. Some
>in house very simple event mechanism should do it.

Yes - we need to move torwards an event/threaded architecture. Async is good.

-D
--
<phone> i am a sausage fan

Jacob Potter
2005-10-31, 16:39
On 10/31/05, fred @ thomascorner. com
<fredthomascorner.com.1xsh6b (AT) no-mx (DOT) forums.slimdevices.com> wrote:
> A hash of some sort looks like the ideal standard command and/or
> response format. In the CLI I will have to maintain the order of
> parameters and responses, but I can't see a way out of it except with
> ordered hashes. RPC/web/player probably care less about that.

I was thinking that it would be possible to translate from the CLI
foo:bar notation to an array/hash system; for example (trimmed
slightly):

"playerindex:0 ip:127.0.0.1:60488 model:softsqueeze playerindex:1
ip:192.168.1.22:3483 model:slimp3"

.... would become:
{ "players" => [
{ "ip" => "127.0.0.1:60488", "model" => "softsqueeze" },
{ "ip" => "192.168.1.22:3483", "model" => "slimp3" }
] }

The basic commands such as play, pause, etc. would translate to
"command", "subcommand", "value" (for mixer), "start", "end", etc.

The RPC plugin currently has two layers; there's the actual JSON/XML
handling, and then a mini command layer with operations such as
getPlaylist, getStrings, and doCommand. The latter simply forwards to
Slim::Control::Command::execute(). It'd be a big improvement to have a
hash-based command structure internally, which could then be
translated as needed for CLI, RPC, xPL, whatever.

Make sense?

- Jacob

dean
2005-10-31, 16:54
On Oct 31, 2005, at 3:39 PM, Jacob Potter wrote:

> On 10/31/05, fred @ thomascorner. com
> <fredthomascorner.com.1xsh6b (AT) no-mx (DOT) forums.slimdevices.com> wrote:
>
>> A hash of some sort looks like the ideal standard command and/or
>> response format. In the CLI I will have to maintain the order of
>> parameters and responses, but I can't see a way out of it except with
>> ordered hashes. RPC/web/player probably care less about that.
>>
>
> I was thinking that it would be possible to translate from the CLI
> foo:bar notation to an array/hash system; for example (trimmed
> slightly):
>
> "playerindex:0 ip:127.0.0.1:60488 model:softsqueeze playerindex:1
> ip:192.168.1.22:3483 model:slimp3"
>
> ... would become:
> { "players" => [
> { "ip" => "127.0.0.1:60488", "model" => "softsqueeze" },
> { "ip" => "192.168.1.22:3483", "model" => "slimp3" }
> ] }
The unfortunate thing about the players command response is that
there are multiple instances of the same variable name, broken up by
a specific variable name (playerindex). This means that the creation
of the cli format and subsequent parsing needs to know about specific
variable names, rather than have this be part of the syntax. The
titles, songs and tracks commands suffer similarly, except that they
don't have a defined separator, or do they?

> The basic commands such as play, pause, etc. would translate to
> "command", "subcommand", "value" (for mixer), "start", "end", etc.
Right, but do note that we'd still (for backwards compaitiblity) need
to allow folks to use the existing array reference structure for
executing commands. How do we do this?

Jacob Potter
2005-10-31, 18:55
On 10/31/05, dean blackketter <dean (AT) slimdevices (DOT) com> wrote:
> The unfortunate thing about the players command response is that
> there are multiple instances of the same variable name, broken up by
> a specific variable name (playerindex). This means that the creation
> of the cli format and subsequent parsing needs to know about specific
> variable names, rather than have this be part of the syntax. The
> titles, songs and tracks commands suffer similarly, except that they
> don't have a defined separator, or do they?

I think I put my example the wrong way around. A CLI notation <-> hash
notation gateway would only need to translate /commands/ from the old
string system, and for those, the syntax is much more well-defined.
Responses would be much easier to handle, since it's going from a
"more specific" to a "less specific" format.

> Right, but do note that we'd still (for backwards compaitiblity) need
> to allow folks to use the existing array reference structure for
> executing commands. How do we do this?

The array -> hash parsing could be done within the core command code,
rather than handling it in the CLI plugin, Does anything within the
server use the response from Command::execute()?

- Jacob

Fred
2005-10-31, 19:06
>> A hash of some sort looks like the ideal standard command and/or
>> response format. In the CLI I will have to maintain the order of
>> parameters and responses, but I can't see a way out of it except with
>> ordered hashes. RPC/web/player probably care less about that.
>
> I was thinking that it would be possible to translate from the CLI
> foo:bar notation to an array/hash system; for example (trimmed
> slightly):
>
> "playerindex:0 ip:127.0.0.1:60488 model:softsqueeze playerindex:1
> ip:192.168.1.22:3483 model:slimp3"
>
> ... would become:
> { "players" => [
> { "ip" => "127.0.0.1:60488", "model" => "softsqueeze" },
> { "ip" => "192.168.1.22:3483", "model" => "slimp3" }
> ] }[/color]
The unfortunate thing about the players command response is that
there are multiple instances of the same variable name, broken up by
a specific variable name (playerindex). This means that the creation
of the cli format and subsequent parsing needs to know about specific
variable names, rather than have this be part of the syntax. The
titles, songs and tracks commands suffer similarly, except that they
don't have a defined separator, or do they?


Jacob -- got it right this time I hope :-)

In your example, my problem is that ip comes before players. By just getting the hash keys, I have no garantee of that. If all the hashes for each player are identical in terms of keys, I hope I would get the same key order every time but I don't think this is even guaranteed.

And yes, there is the problem of "playerindex". The reason is that it's much more simpler to parse if you know what is the "item separator" (you have to do $i++ when you get it). It needs to come before the rest of the stuff for the item, and it allows to return more or less fields per item depending on the need (for example when returning song data, the CLI only returns info for the tags that have a value). And it allows to add fields without breaking the clients that should just ignore it. There is simply no way to make a deterministic parser if you do not enforce an item separator OR a deterministic field list.

When possible, the item separator was selected amongst the fields. In general for DB data it is the item id. For playlist however, the index is a special field (the position of the song in the playlist). Don't remember why I added "playerindex" though (the player id would have done it), but there must be a reason.

Now we could change that but in any case we would have to find a way for legacy...
The other problem is that the order matters in simple queries, so I would have to know what to map the "?" to. And so far, the CLI echoes the parameters in the same order they were received. So when I get:

titles 0 10 artist_id:22 tags:u charset:whatever

even though internally it makes no difference that charset is last and tags in the middle, I always return the stuff in the same order. Again this is to make the parser simpler at the other end (a string compare does the job of knowing if this is your reply, rather than having to decompose the query and check each param again). FYI when listen is 1, there are asynchronous stuff being sent to the client so it needs a way to know it's original command is done. And of course, the likes of "display ? ?" really need to cater for parameters having a definite order.



> The basic commands such as play, pause, etc. would translate to
> "command", "subcommand", "value" (for mixer), "start", "end", etc.[/color]
Right, but do note that we'd still (for backwards compaitiblity) need
to allow folks to use the existing array reference structure for
executing commands. How do we do this?


There are two areas we could investigate (mentally I mean):

- Introduce something like IxHash or whatever CPAN or home made flavour of ordered hash. We probably need "tie". Not sure if we can support that in slimserver (perl version bla bla). Not sure if they would be transparent to the people writing the commands and queries. Not sure that is efficient (but for commands and CLI, do we care?).

- Decide that there is an internal format (that we still need to finalize but have a good idea of) and external formats (the CLI is a well defined one, as is xPL) and that it is the job of the "protocol" or "client controller" to sort out it's external format issues. That means mapping tables/hashes between the internal and the external world.
So for the CLI, I would remember the original command with order and everything, create a correct internal command/query, let the baby run and when it comes back apply some command by command logic to create the CLI reply. It insulates the external world from the internal one (the identifiers could be different, for example), and it is my experience that it generally is a good thing in the long run. Right now I envision the mapping definition to be an array of hashes, array for the position and the hash to map internal to external identifier or something.
And yes, this is exactly what is done for the web ui, which has layers of abstraction at nauseam (between the level stuff and the skin stuff).

Now if we have a nice asynchronous command mechanism, we could add things to the hash like command priority, timing info, result code, etc, and then maybe linearizing all that and sending it out may not be the best solution for RPC.

Comments welcomed, as always

Fred

Michaelwagner
2005-10-31, 22:04
Each of those "elsif ($p0 eq "commandname")" clauses should be moved to separate functions which are looked up in a hash by the new execute().Doesn't perl have a case statement?

Dan Sully
2005-10-31, 22:07
* Michaelwagner shaped the electrons to say...

> Doesn't perl have a case statement?

No.

There is: http://search.cpan.org/~dconway/Switch-1.00/Switch.pm

But you're better off using a dispatch table. The code becomes cleaner and
more modular that way.

-D
--
It does not do to leave a live Dragon out of your calculations..

cdoherty
2005-11-01, 11:22
On Mon, Oct 31, 2005 at 09:07:58PM -0800, Dan Sully said:
> There is: http://search.cpan.org/~dconway/Switch-1.00/Switch.pm
>
> But you're better off using a dispatch table. The code becomes cleaner and
> more modular that way.

I'm working on this, btw--a lot of it is making sure the returned array of
the new execute() looks like that from the old execute(). on the scale of
things it doesn't look super-hard, just hairy.

I'll post a patch as soon as I have some basic functionality for a few of
the less complex commands.

chris


-------------------------------
Chris Doherty
chris [at] randomcamel.net

"I think," said Christopher Robin, "that we ought to eat
all our provisions now, so we won't have so much to carry."
-- A. A. Milne
-------------------------------

Michaelwagner
2005-11-01, 11:31
It does not do to leave a live Dragon out of your calculations..I love this quote ...

Fred
2005-11-01, 15:33
I'm working on this, btw--a lot of it is making sure the returned array of
the new execute() looks like that from the old execute(). on the scale of
things it doesn't look super-hard, just hairy.


Chris,

we're talking about changing or not even using any longer the returned array of execute, so you may not need to bother about it...

What is it exactly that you're working on?

We need to stay coordinated

Thanks

Fred

Fred
2005-11-01, 15:35
Does anything within the
server use the response from Command::execute()?

Yes. The CLI mainly unless listen is on and I think for "playlist save" or something.

Fred

cdoherty
2005-11-04, 10:49
On Tue, Nov 01, 2005 at 02:33:03PM -0800, fred @ thomascorner. com said:
> we're talking about changing or not even using any longer the returned
> array of execute, so you may not need to bother about it...
>
> What is it exactly that you're working on?

just pulling out the individual commands into a function lookup table, to
make execute(), um, readable. 1 command == 1 function (and some functions
have their own little sub-lookup-tables).

AFAICT this will need to be done as long as command execution goes through
execute(), regardless of what other kinds of work goes on.

no?

chris


-------------------------------
Chris Doherty
chris [at] randomcamel.net

"I think," said Christopher Robin, "that we ought to eat
all our provisions now, so we won't have so much to carry."
-- A. A. Milne
-------------------------------

Fred
2005-11-04, 16:34
[Note: this is the same Fred as before, just changed my forum name to avoid spam...]

Chris,

Well, what I was thinking of was:

- fully separate queries from commands.
- use some form of hash (TBD) for the parameters
- use some form of hash for the responses
- go through idle() for processing (TBD) as well
- patch execute to call the new system for existing code

I guess what you're working on is a start. When do you think you will be done and how do you keep up with the commits?

Fred

cdoherty
2005-11-04, 17:09
On Fri, Nov 04, 2005 at 03:34:22PM -0800, Fred said:
> Well, what I was thinking of was:
>
> - fully separate queries from commands.
> - use some form of hash (TBD) for the parameters
> - use some form of hash for the responses
> - go through idle() for processing (TBD) as well
> - patch execute to call the new system for existing code

I think I'm more narrowly and short-term focused than the discussion so
far. execute() as it currently stands is (IMO) a harsh starting point for
the kind of major shifts you're aiming for, and I think pulling the logic
of its commands out into separate functions will be a necessary step in
any of the kinds of changes you're talking about. I think they're good
ideas: storing the results as an intermediate data structure, having some
modular way to convert those results into an output format, all good
things.

my goal is only to pull that command code out and turn execute() into a
function lookup--ideally, nothing outside Command.pm will notice any kind
of change. (though as I mentioned in the code, if there's a consensus that
the return value of execute() is irrelevant, that would make me smile a
lot.)

seem reasonable?

> I guess what you're working on is a start. When do you think you will
> be done and how do you keep up with the commits?

now that I'm more familiar with the server environment and the tools, I
think it can go more quickly. if everyone (or enough of the right people)
agrees that the gold standard of compatibility is that the old and new
versions perform the same actions and return the same data--that's
something pretty concrete to test against, and the commands can be
switched over one at a time.

by "keep up with the commits", do you mean keeping up with the changes
people are making to the command code?

chris





-------------------------------
Chris Doherty
chris [at] randomcamel.net

"I think," said Christopher Robin, "that we ought to eat
all our provisions now, so we won't have so much to carry."
-- A. A. Milne
-------------------------------

Fred
2005-11-04, 17:57
if there's a consensus that
the return value of execute() is irrelevant, that would make me smile a
lot.
The return value is needed for now.

seem reasonable?
yes


by "keep up with the commits", do you mean keeping up with the changes
people are making to the command code?

Yes.

Fred




-------------------------------
Chris Doherty
chris [at] randomcamel.net

"I think," said Christopher Robin, "that we ought to eat
all our provisions now, so we won't have so much to carry."
-- A. A. Milne
-------------------------------[/QUOTE]

dean
2005-11-05, 19:37
I think that this is the best first step.

Do note that return value of execute is used to return values from
the execute function.


On Nov 4, 2005, at 4:09 PM, Chris Doherty wrote:
> my goal is only to pull that command code out and turn execute()
> into a
> function lookup--ideally, nothing outside Command.pm will notice
> any kind
> of change. (though as I mentioned in the code, if there's a
> consensus that
> the return value of execute() is irrelevant, that would make me
> smile a
> lot.)
>
> seem reasonable?

Fred
2005-11-06, 19:23
Chris,

Looked up the new code. Pretty nice. Comments:
A. Why do you go through eval rather than use a reference directly in the hash?
B. You need to find a way to undef $client when returning
C. The callback(s) is (are) missing

Again, I would like to introduce changes to the model now rather than once we have 45 functions to change once we decide on a new parameter mechanism. We all know the array business is a problem so why don't we fix it now ? Returning the array is a problem so why don't we get rid of that ?


Fred

cdoherty
2005-11-07, 00:21
On Sun, Nov 06, 2005 at 06:23:40PM -0800, Fred said:
> Looked up the new code. Pretty nice. Comments:
> A. Why do you go through eval rather than use a reference directly in
> the hash?

it simplifies the hash, and was just how it came out when I approached the
problem from the "make lots of little functions" angle. it could be
references or fully-qualified package+function names.

> B. You need to find a way to undef $client when returning

for the transition case, yes, where one of the if..else has been replaced
by a call to execute_new(). I was thinking of something aesthetically
unpleasant like passing \$client to execute_new(); then again, maybe we
could make execute_new() be execute() and just pass to execute_old()
anything not yet implemented in the new scheme. (I think I like the
\$client idea better, unfortunately.)

suggestions welcome.

> C. The callback(s) is (are) missing

yah, that's in the current round (I haven't had any time this weekend).

> Again, I would like to introduce changes to the model now rather than
> once we have 45 functions to change once we decide on a new parameter
> mechanism. We all know the array business is a problem so why don't we
> fix it now ? Returning the array is a problem so why don't we get rid
> of that ?

that's a wider architectural question than I'm capable of answering at
this point.

chris



-------------------------------
Chris Doherty
chris [at] randomcamel.net

"I think," said Christopher Robin, "that we ought to eat
all our provisions now, so we won't have so much to carry."
-- A. A. Milne
-------------------------------

Fred
2005-11-07, 19:41
Ok, here's a very rough initial proposal to expose the principles. For it to work you need Tie::LLHash from CPAN.

I introduce a new object named "Cmd". This holds the entire command data. It uses a couple of ordered hashes to keep things simple for existing code and the CLI. The Cmd object provides methods to manipulate its data. It provides an "execute" method that goes through a "dispatch" function in Command.pm. There is no fundamental difference between it and what Chris was proposing, I just did another one in order to leave his code intact.

In CLI.pm, I have added a "test" command that shows how to use the new object. Ideally all code should move to something like that.

In Command.pm, I have added (and commented out) stub code that creates a Cmd out of the parameters, executes it, and recreates the resultArray back. If you uncomment it, you can try going through it for the "pref" command or query from the CLI and it works. The trick here is the ordered hash...

In Execute.pm. there are 2 new functions to manage the pref command and query, respectively. I hope one can clearly see what are their parameters just by looking at their first lines.

Now this does not handle callbacks either, but that would be another data point of the Cmd object handled by dispatch. In fact, Cmd->execute could even post the cmd to some queue that is processed during idle (callbacks would be mandatory at that point if the caller cares about the result of the command or query).
We would also need to separate commands from queries with a better trick than adding 'q' to the command name as I have done here for simplicity reasons: 2 different dispatch functions or 2 hashes and/or even maybe a subclass of Cmd for Queries. Complex queries will need something more complex than the simple hash that is implemented for the moment.

I think it makes the whole thing simpler while maintaining a pretty good compatibility path with the existing stuff. I had a look at the 200 or so calls to execute in the current code base and most of it is "stop" and "power 1", so that should not cause any problems.

Of course, keep in mind this was done in a couple of hours so there is a lot of polish to add, but if we all agree I think the resulting function library will be much more generic, which was the objective of this thread :-)

Comments welcomed.

Fred

Dan Sully
2005-11-07, 20:28
* Fred shaped the electrons to say...

>Ok, here's a very rough initial proposal to expose the principles. For
>it to work you need Tie::LLHash from CPAN.

Why?

>I introduce a new object named "Cmd". This holds the entire command
>data. It uses a couple of ordered hashes to keep things simple for
>existing code and the CLI. The Cmd object provides methods to
>manipulate its data. It provides an "execute" method that goes through
>a "dispatch" function in Command.pm. There is no fundamental difference
>between it and what Chris was proposing, I just did another one in order
>to leave his code intact.

I like the general idea. Some comments on naming, etc.

Can we call it something else? Cmd seems shortend for no reason other than to
not conflict with the current Slim::Control::Command (which I argue isn't
named well to begin with). What is the purpose of this object? Naming should
follow the purpose.

Also - I find it easier to write:

return if !$cmd->getCommand() eq 'pref';

as:

return if $cmd->getCommand() ne 'pref';

to avoid the negation.

Good work, Fred.

-D
--
<Djall> and I also learned that a meat vortex takes meat away from you.

Fred
2005-11-08, 05:08
>>Ok, here's a very rough initial proposal to expose the principles. For
>>it to work you need Tie::LLHash from CPAN.[/color]

>Why?

Tie::LLHash maintains the order of insertion in the hash. Some CLI calls are positional, so their order is important; the spec says the server "echoes" the commands, i.e. maintaining ordering.
Even in extended CLI calls, today I maintain the order, it makes parsing the response for the client simpler. I.e. when issuing:
genres 0 3 artist:3 album:4 charset:utf8
the CLI keeps the order of artist, album and charset in the reply, and then starts with "count".

Practically if you look at the stub code for execute(), I am rebuilding the array from the Cmd object. I can only do that because ordering is maintained. The code I propose would even work for "display ? ?", providing the function handling it in Execute.pm adds results to the array in the correct order...

I hope the above answers the "why maintain the insertion order" bit of the question. Now we can do our own thing if LLHash does not do it, or CPAN proposes IxHash as well to do the same thing.

>Can we call it something else? Cmd seems shortend for no reason other
>than to not conflict with the current Slim::Control::Command (which I
>argue isn't named well to begin with). What is the purpose of this object?
>Naming should follow the purpose.

You're correct in that I chose Cmd just to avoid the conflict. Now to me, the purpose of the object is to define and execute Commands and Queries, so maybe Request is a better term?
Any better ideas?

Re. Command.pm, there will be little left of it once this work is completed, but we will need to keep it around just to make sure "Slim::Control::Command::execute()" functions still work (in plugins, f.e.).

Dean, Chris, Jacob, your comments would be appreciated as well.

Fred

dean
2005-11-08, 12:53
On Nov 7, 2005, at 7:28 PM, Dan Sully wrote:
> Also - I find it easier to write:
>
> return if !$cmd->getCommand() eq 'pref';
Ew. This is scary because it requires you to think about operator
precedence. Even now, I'm not sure it's doing the right thing...

> as:
> return if $cmd->getCommand() ne 'pref';
How about:

if ($cmd->getCommand() ne 'pref') {
return;
}

It's a little harder to write, but it's a lot easier to _read_.

-dean

Fred
2005-11-08, 14:32
Guys,

I think we need a sticky "Slimserver Perl Style Guide" post on the forum :-)

Now considering we'll have about 40 or so functions with the same line, I don't think it matters a lot since all you'll be looking for is the function "name" (i.e. perf) to see if it is right, the rest would come from a copy paste...

Any further comment? In particular about the architecture? Names? Should I commit something like described below and we would start from there?

I'd propose to create a Dispatch.pm file with the dispatcher routine in it, including callback management, provision to add commands/queries to the hashes (to be used by plugins).
Commands and Queries would go in Commands.pm and Queries.pm, respectively.
The renamed (and improved for Query support) "Cmd" class could go in Request.pm.

Once this base is in place we can all happily improve away.

Or does anybody have a better suggestion?

Thanks

Fred

Michaelwagner
2005-11-08, 14:38
I think we need a sticky "Slimserver Perl Style Guide" post on the forum :-)Doesn't the Wiki already have one? In which case, the post in the forum should just send one to the wiki page. No point in needlessly duplicating....

Dan Sully
2005-11-08, 14:38
* Fred shaped the electrons to say...

>I think we need a sticky "Slimserver Perl Style Guide" post on the
>forum :-)

Yes - forthcoming.

>I'd propose to create a Dispatch.pm file with the dispatcher routine in

Dispatch is much better.

-D
--
<iNoah> kernel's original recipe: 11 secret args and switches

Dan Sully
2005-11-08, 14:42
* Michaelwagner shaped the electrons to say...

> Doesn't the Wiki already have one? In which case, the post in the forum
> should just send one to the wiki page. No point in needlessly
> duplicating....

It does not - but it will, and the Forum post will point there.

-D
--
It is dark. You are likely to be eaten by a grue.

dean
2005-11-08, 15:43
On Nov 8, 2005, at 1:32 PM, Fred wrote:
> I think we need a sticky "Slimserver Perl Style Guide" post on the
> forum :-)
An excellent idea. But better in the Wiki...

> Now consid

Fred
2005-11-09, 18:32
Should I consider the lack of comments about the background of my proposal as agreement to proceed?

It's a lot of work I rather not do if is not what we want...

Fred

cdoherty
2005-11-09, 19:04
On Tue, Nov 08, 2005 at 01:32:13PM -0800, Fred said:
> Any further comment? In particular about the architecture? Names?
> Should I commit something like described below and we would start from
> there?
>
> I'd propose to create a Dispatch.pm file with the dispatcher routine in
> it, including callback management, provision to add commands/queries to
> the hashes (to be used by plugins).
> Commands and Queries would go in Commands.pm and Queries.pm,
> respectively.
> The renamed (and improved for Query support) "Cmd" class could go in
> Request.pm.

er, are queries handled through execute() currently? how do queries differ
from commands such that they need to be handled differently?

do I still need to refactor execute(), or does this make that work
obsolete before it's happened? is the Brave New World maintaining
compatibility with execute()'s return array, or is this the server-wide
change everyone has been dreaming of?

chris


-------------------------------
Chris Doherty
chris [at] randomcamel.net

"I think," said Christopher Robin, "that we ought to eat
all our provisions now, so we won't have so much to carry."
-- A. A. Milne
-------------------------------

Fred
2005-11-09, 20:01
er, are queries handled through execute() currently? how do queries differ
from commands such that they need to be handled differently?


Yes (some of them are in CLI.pm as well). You coded one doing 'pref'. When issued as 'pref composerInArtists ?', it's a query. It does not do anything, it is just used to return the value of the preference. This is one of the reasons why the array must be returned in the current architecture, it's the only way the caller gets to have its answer!

The reason I propose to handle them differently is because of callbacks. There are a few subscribers to the execute callback. (Note: there are two callbacks possible in execute: one is given by the caller to be notified when its command has completed. The other is a global notification mechanism where one can "subscribe" to all things happening in the server. You can try it with "listen 1" in the CLI.)
With the current architecture, queries are "broadcast" as notifications, but this is really not necessary (nothing changed). In the CLI as well as in xPL, there is an attempt (or an ambition) at providing asynchronous notifications of changes to clients. Clearly this is impossible to do cleanly if you cannot distinguish between some guy asking about the preference or some guy changing it (the return array is the same in both cases:
Query case: "pref composerInArtists ?" sent, "pref composerInArtists 1" returned (and broadcast). Command case: "pref composerInArtists 1" sent, "pref composerInArtists 1" returned and broadcast.

By separating them, we can distinguish between both and only broadcast commands (so that listeners know something changed or at least was acted upon). Should we have finer control over multitasking, one could argue queries are lower priority than commands. It separates code into logical parts whereas now Command.pm contains queries only (version ?), command/queries combinations (pref) and commands only (stop).
I have other ideas as well, for another day...



do I still need to refactor execute(), or does this make that work
obsolete before it's happened?


Refactoring execute is needed. Have a look at the patch if you can, I'm just going beyond what you propose, and hence what you were proposing remains to be done, if only a bit differently. I did a copycat of your execute hash for my demonstrations needs, but I would welcome your help on this bit.



is the Brave New World maintaining
compatibility with execute()'s return array, or is this the server-wide
change everyone has been dreaming of?


Again, have a look at the patch. It contains a version of execute() that maintains compatibility for those sections of code we will not be able to change (i.e. that will still call execute despite the existence of our shiny new better mechanism).

Maybe I can work on this tomorrow and commit something so it's clearer for everyone how that works and where each can help.

Fred

Fred
2005-11-13, 19:47
OK, done as discussed in rev 5183. This seems to work for me, but I have not tested everything.
The Request class is still a bit rough at the moment, comments welcomed.

Fred

Fred
2006-01-04, 20:02
It took a while but execute() in the latest svn is empty, save for the glue code that creates a proper request object, executes it and re-creates a proper array for any callback clients to be happy.

There remains some queries in CLI.pm to re-move into Queries.pm. Dispatch and Request must be adapted to support callbacks and notifications. Then CLI.pm will be migrated to "native" Dispatch and following that, all callers to execute. Somewhere down this road plugins will need to be able to add their calls to the dispatch table.

As usual, comments more than welcomed. In particular about the latter, or about the Request API you'd like to see for clients...

Fred

cdoherty
2006-01-04, 22:41
On Wed, Jan 04, 2006 at 07:02:40PM -0800, Fred said:
> It took a while but execute() in the latest svn is empty, save for the
> glue code that creates a proper request object, executes it and
> re-creates a proper array for any callback clients to be happy.

yay Fred! thanks for all the hard work.

chris


-------------------------------
Chris Doherty
chris [at] randomcamel.net

"I think," said Christopher Robin, "that we ought to eat
all our provisions now, so we won't have so much to carry."
-- A. A. Milne
-------------------------------

Michaelwagner
2006-01-05, 06:27
I add my cheers.

I was looking at execute a while ago because I'm trying to think about running a very slim server on a slug, and I was trying to figure out how that area worked. Execute was quite lengthly, essentially a huge case statement (without actually being a case statement).

I think when the server is split into multiple threads, in the next major release, a clear execute is going to be a great asset.

Michaelwagner
2006-01-08, 20:53
It took a while but execute() in the latest svn is empty, save for the glue code that creates a proper request object, executes it and re-creates a proper array for any callback clients to be happy.
Hi Fred:

Did this go into 6.2.2 or only 6.5?

'cuz I'm not seeing it here in the copy of 6.2.2 I downloaded only yesterday.

Fred
2006-01-09, 03:17
This is a pretty major code change. It's on 6.5 only...

Fred

Michaelwagner
2006-01-09, 07:52
Fred:

Ah, OK, thanks, that's why I'm not seeing it.

Perhaps I should take the time to figure out how to run 2 code bases at the same time ... 3 I suppose ... at the moment, 6.2.1, 6.2.2 and 6.5

Michael

Fred
2006-01-09, 10:58
Why "run" 3? I do have latest release in "production" (on a "production" machine), and 6.2.x and 6.5 as directories on a "test" machine (with a test DB). I use Softsqueeze for most mundane testing (like does it compile at all...). When the time comes for better testing, I do change my players to target the dev running code.

If some behaviour sounds strange, I CLI the production machine to see what's the behaviour there (or use its web interface or whatever).

Only issue is that unless cleverly using the command line options when starting the server (I think, never bothered to investigate that), it gets all confused when starting up 6.2 after 6.5 so I have to go and clean the caches manually.

Of course this is on OS X, there may be other issues on Windows and/or if you have a single prod/dev machine...

Fred

Michaelwagner
2006-01-10, 07:34
I guess I should have said "keep 3 sources online".

But I probably need to be able to switch which one is running in order to test things.

At the moment, I run 6.2.2 because it fixed some things I needed fixed.

But reproducing problems in order to report them means I should also have a running copy of 6.2.1, because that's the official release.

Windows does make this a bit complicated because there is one that starts at boot time and that is started by slimtray. I guess I have to look at slimtray to figure out how it does that, and if there's an option to start/stop multiple ones.

Maybe I can also figure out how a function that's worth about 15 KB of storage can take 8MB :-)

Fred
2006-01-13, 17:28
For the record, I consider the migration finished, except for some documentation I still have to write in the code and maybe on the wiki.

Fred

Dan Sully
2006-01-13, 17:40
* Fred shaped the electrons to say...

>For the record, I consider the migration finished, except for some
>documentation I still have to write in the code and maybe on the wiki.

Fred - this is all fantastic work!

Thanks so much.

-D
--
You know, for kids.

Triode
2006-01-13, 18:04
Fred,

xPL crashes if it is notified by something not setting client - basically because its looking for client to be undef not 0 in this
case:

Will client always be 0 if no client is set? If so xPL and anything else could easily be changed, but would undef be more natural?

frame 0: Plugins::xPL::xplExecuteCallback (/usr/local/slimserver/Slim/Control/Request.pm line 1075)
frame 1: Slim::Control::Request::notify (/usr/local/slimserver/Slim/Control/Request.pm line 994)
frame 2: Slim::Control::Request::execute (/usr/local/slimserver/Slim/Control/Request.pm line 457)
frame 3: Slim::Control::Request::executeRequest (/usr/local/slimserver/Slim/Web/Setup.pm line 1338)
frame 4: Slim::Web::Setup::__ANON__ (/usr/local/slimserver/Slim/Web/Setup.pm line 2824)
frame 5: Slim::Web::Setup::processChanges (/usr/local/slimserver/Slim/Web/Setup.pm line 2565)
frame 6: Slim::Web::Setup::setup_HTTP (/usr/local/slimserver/Slim/Web/HTTP.pm line 744)
frame 7: Slim::Web::HTTP::generateHTTPResponse (/usr/local/slimserver/Slim/Web/HTTP.pm line 653)
frame 8: Slim::Web::HTTP::processURL (/usr/local/slimserver/Slim/Web/HTTP.pm line 513)
frame 9: Slim::Web::HTTP::processHTTP (/usr/local/slimserver/Slim/Networking/Select.pm line 117)
frame 10: Slim::Networking::Select::select (./slimserver.pl line 626)
frame 11: main::idle (./slimserver.pl line 562)
frame 12: main::main (./slimserver.pl line 1226)

$VAR1 = 0; <<<< dump of $client in xPL callback
Can't call method "name" without a package or object reference at /usr/local/slimserver/Plugins/xPL.pm line 624.

[this was triggered by performing a rescan]

Adrian

----- Original Message -----
From: "Fred" <Fred.21lmfz (AT) no-mx (DOT) forums.slimdevices.com>
To: <developers (AT) lists (DOT) slimdevices.com>
Sent: Saturday, January 14, 2006 12:28 AM
Subject: [Developers] Re: CLI versus RPC


>
> For the record, I consider the migration finished, except for some
> documentation I still have to write in the code and maybe on the wiki.
>
> Fred
>
>
> --
> Fred
> ------------------------------------------------------------------------
> Fred's Profile: http://forums.slimdevices.com/member.php?userid=2170
> View this thread: http://forums.slimdevices.com/showthread.php?t=15973
>
>

Fred
2006-01-13, 18:30
Trying to figure out which ^#%#$ is using 0 as a client, deep into Setup.pm probably.

I am working on it. Noticed it on my own doing some testing...

Thanks

Fred

Triode
2006-01-13, 18:35
Setup line 2552? in setup_HTTP

Triode
2006-01-13, 18:47
Fred - Above worked for me, I've put it in svn.

Adrian

Fred
2006-01-13, 18:55
Saw your commit. My commit (of the same correction following the same hunt) failed because of yours :-)

Thanks

Fred