CHDK PTP interface - page 58 - General Discussion and Assistance - CHDK Forum
supplierdeeply
« previous next »
Pages: 1 ... 53 54 55 56 57 [58] 59 60 61 62 63 ... 125   Go Down

CHDK PTP interface

  • 1241 Replies
  • 488081 Views
*

Offline reyalp

  • ******
  • 14080
Re: CHDK PTP interface
« Reply #570 on: 05 / September / 2011, 18:35:22 »
Advertisements
I see philmoz added live view support in trunk changeset 1316

This is useful, commonly requested functionality. It has sat on the back burner for far to long, and that's mostly my fault. I appreciate the effort both mweerden and philmoz have put into it.

That said, the implementation leaves me shaking my head.

Why is there PTP code in luascript.c (so lua can return a pointer to a static I guess but ...) ?
Why are we overloading PTP_CHDK_CallFunction with new magic behavior ?
Why are we passing a single hard coded address of a function from Lua (in a struct stuffed into a binary string), to the PTP client just so the PTP client can just pass it back for CHDK to call it ? How is this preferable than just making another CHDK PTP opcode that calls the function ? We have 2^32 available...
Why are we involving Lua at all when we are just passing back a hard-coded binary blob ? Sure, some of these values might be useful for other things, but this is not a very usable format. If that blob ever changes, it will be an incompatible protocol revision.
Why are there magic numbers all over the place ?

Guess this is what I get for not looking at it sooner...  :(

I understand the desire to get something working, but we've already reached the point where CHDK is nearly unmaintainable due to people piling on half baked hacks. We really need a development branch or something where we can refine larger features like this without worrying too much about compatibility in the intermediate revisions.
Logged
Don't forget what the H stands for.
*

Offline philmoz

  • *****
  • 3450
    • Photos
Re: CHDK PTP interface
« Reply #571 on: 05 / September / 2011, 20:33:11 »
Quote from: reyalp on 05 / September / 2011, 18:35:22
I see philmoz added live view support in trunk changeset 1316

This is useful, commonly requested functionality. It has sat on the back burner for far to long, and that's mostly my fault. I appreciate the effort both mweerden and philmoz have put into it.

That said, the implementation leaves me shaking my head.

Why is there PTP code in luascript.c (so lua can return a pointer to a static I guess but ...) ?
Why are we overloading PTP_CHDK_CallFunction with new magic behavior ?
Why are we passing a single hard coded address of a function from Lua (in a struct stuffed into a binary string), to the PTP client just so the PTP client can just pass it back for CHDK to call it ? How is this preferable than just making another CHDK PTP opcode that calls the function ? We have 2^32 available...
Why are we involving Lua at all when we are just passing back a hard-coded binary blob ? Sure, some of these values might be useful for other things, but this is not a very usable format. If that blob ever changes, it will be an incompatible protocol revision.
Why are there magic numbers all over the place ?

Guess this is what I get for not looking at it sooner...  :(

I understand the desire to get something working, but we've already reached the point where CHDK is nearly unmaintainable due to people piling on half baked hacks. We really need a development branch or something where we can refine larger features like this without worrying too much about compatibility in the intermediate revisions.

Hi reyalp,

Thanks for the feedback, here's a summary of some of the discussion I had with mweerden.

I can't comment on the PTP_CHDK_CallFunction, that was there when I started looking at it (I assumed this was something already worked out).

Putting the live view code in Lua makes sense to me (mweerden convinced me of this).
Almost everything else done in regards to controlling the camera via PTP is done via Lua code - it made sense to do this the same way rather than put the live view as a special PTP function.

The current structure is pretty localised in terms of changes to CHDK, and minimises the number of calls between the client and camera to get the live view info (I played around with different options; but this was the fastest in terms of achievable frame rate). I'm not sure how else you could do something like this without passing data structures around - there's a reasonable amount of information required and some of it can change between calls.

I agree changes to the data structures could cause problems if not done carefully; but, short of using XML, I don't know how else you could do this.

I tried sending back the viewport/bitmap/palette buffer addresses and then using the PTP 'get memory' call to retrieve the data - but it was a lot slower for some reason. Plus there's more chance the buffer address will change between the time the client gets it and the time it uses it to get the data.

Not sure what magic numbers you are referring to; but these can always be changed to constants if it makes the code more readable?

None of this is set in stone, there are no real clients using it - the goal was to get something that was being used and tested to a point where active development on clients could proceed without having to constantly patch the source. A lot of testing was done by nstatic on the SX130 as well as my testing on the G12/SX30/IXUS310. This could have been in a different branch; but then you have to deal with the metadata mess in SVN every time you want to merge.

There's still plenty of scope for improving the way this is done.

I'd actually consider this to be a reasonably small change, it's localised and (apart from the PTP change) doesn't affect anything else. The amount of extra code is also pretty small.

Phil.

Logged
CHDK ports:
  sx30is (1.00c, 1.00h, 1.00l, 1.00n & 1.00p)
  g12 (1.00c, 1.00e, 1.00f & 1.00g)
  sx130is (1.01d & 1.01f)
  ixus310hs (1.00a & 1.01a)
  sx40hs (1.00d, 1.00g & 1.00i)
  g1x (1.00e, 1.00f & 1.00g)
  g5x (1.00c, 1.01a, 1.01b)
  g7x2 (1.01a, 1.01b, 1.10b)
*

Offline mweerden

  • ***
  • 194
Re: CHDK PTP interface
« Reply #572 on: 05 / September / 2011, 21:37:45 »
My two cents:

It seems your main point is about adding functionality to Lua vs. adding it to CHDK PTP. At least, most of the big things you question seem to be a consequence of this choice.

I think the rationale has been given in the past, but it comes down to me having preferred a minimal CHDK PTP. And a large argument for that is that adding more to it makes incompatible protocol revisions more likely. The new functionality is not a part of CHDK PTP, just something that is used through it.

Below some more detailed answers to the specific questions.

Quote from: reyalp on 05 / September / 2011, 18:35:22
Why is there PTP code in luascript.c (so lua can return a pointer to a static I guess but ...) ?
As said, I don't see it as PTP code. However, there is little consequence (if any) if you wish to move it to a different location.

Quote
Why are we overloading PTP_CHDK_CallFunction with new magic behavior ?
Why is it magic? I believe it's properly defined and on purpose in a way that is non-specific to the remote live view functionality.

Quote
Why are we passing a single hard coded address of a function from Lua (in a struct stuffed into a binary string), to the PTP client just so the PTP client can just pass it back for CHDK to call it ? How is this preferable than just making another CHDK PTP opcode that calls the function ? We have 2^32 available...
The core question, as discussed above.

Quote
Why are we involving Lua at all when we are just passing back a hard-coded binary blob ? Sure, some of these values might be useful for other things, but this is not a very usable format. If that blob ever changes, it will be an incompatible protocol revision.
Apart from what I've said so far, because Lua is (at this moment) the only way we can call a named function.

The format is not very usable from within Lua itself, but the alternatives I saw when I started this were a function that returns a Lua structure with the information or separate functions for each of the values. Both I found less practical in their actual use and the second also meant adding a lot more clutter to Lua. Besides, I'm not sure that use of this information within Lua is ever going to be of any significance.

Changes to "the blob" that will lead to incompatibility with previous revisions will have the same effect in pretty much any other way remote live view can be implemented, I'd think. Unless the actual meaning of certain values has to be adjusted, I believe the code is pretty robust against possible future extensions.

Quote
Why are there magic numbers all over the place ?
I believe "all over the place" isn't entirely fair. In any case, it's not so much criticism on the new features as something that I'm sure can be easily taken care of if asked.

Quote
I understand the desire to get something working, but we've already reached the point where CHDK is nearly unmaintainable due to people piling on half baked hacks. We really need a development branch or something where we can refine larger features like this without worrying too much about compatibility in the intermediate revisions.
And, as happens often, we come to the bigger discussion. I believe I've spoken my mind about it before, so I'll try to restrain myself. Fact is that things are the way they are and they don't seem likely to change significantly any time soon. "Don't forget what the H stands for." comes to mind.
Logged
*

Offline reyalp

  • ******
  • 14080
Re: CHDK PTP interface
« Reply #573 on: 05 / September / 2011, 22:25:54 »
Quote from: philmoz on 05 / September / 2011, 20:33:11
I can't comment on the PTP_CHDK_CallFunction, that was there when I started looking at it (I assumed this was something already worked out).
Fair enough. This feels like the wrong way to do it to me. I can sort of see the argument that you might want other functions that produce a similar stream of data, but passing them around seems suspect to me. Since these functions would have to be defined in CHDK code anyway, it's not as if clients are going to make these up on the fly.

Also, if you did end up with multiple functions like this, you'd want to combine all the data together rather than polling them independently.
Quote
Putting the live view code in Lua makes sense to me (mweerden convinced me of this).
Almost everything else done in regards to controlling the camera via PTP is done via Lua code - it made sense to do this the same way rather than put the live view as a special PTP function.
I'm not convinced of this. It doesn't make sense to handle the live view data (framebuffer/palette etc) in Lua, so passing this one chunk a different way seems needless complexity. You already need a non-lua part on both ends to handle the data, and you are already sending *some* of the information about framebuffer layout with that. If you could sanely do the whole thing in Lua, I'd be all for that.
Quote
I agree changes to the data structures could cause problems if not done carefully; but, short of using XML, I don't know how else you could do this.
If you are going to use Lua, put it in a Lua table. That gives you a nice key/value representation, without the overhead of XML.
Quote
I tried sending back the viewport/bitmap/palette buffer addresses and then using the PTP 'get memory' call to retrieve the data - but it was a lot slower for some reason. Plus there's more chance the buffer address will change between the time the client gets it and the time it uses it to get the data.
This all makes sense. I agree with packing them together like this. In fact, I would like to take this one step further, which I'll get to in another post.
Quote
None of this is set in stone, there are no real clients using it - the goal was to get something that was being used and tested to a point where active development on clients could proceed without having to constantly patch the source.
The problem is the trunk is the build used by end users, so the "not-set-in-stone" bit has the potential to come back and bite you. But I agree, we need something to work from without constantly shuffling patches. My gut reaction aside, it needed to be done :)
Quote
This could have been in a different branch; but then you have to deal with the metadata mess in SVN every time you want to merge.
I've removed the spurious mergeinfos (generally you aren't supposed to mess with it but these were introduced by mistakes in the first place...)
Quote
I'd actually consider this to be a reasonably small change, it's localised and (apart from the PTP change) doesn't affect anything else. The amount of extra code is also pretty small.
Agree this doesn't directly impact anything else. My reaction is to the (IMHO) ugliness of the code.
Quote
Not sure what magic numbers you are referring to
The numbers for the different outputs of handle_video_transfer etc. Understand this is work in progress. Not a big deal, but they should appear as constants in ptp.h along with comments. I'm trying to keep that as the canonical definition of our protocol.


On the up side, I managed to build the dot net app and connect to my d10 without any additional changes :)
Logged
Don't forget what the H stands for.
*

Offline reyalp

  • ******
  • 14080
Re: CHDK PTP interface
« Reply #574 on: 05 / September / 2011, 23:19:54 »
First, thanks for doing this work, and responding to my initial, flamey reaction.
Quote from: mweerden on 05 / September / 2011, 21:37:45
It seems your main point is about adding functionality to Lua vs. adding it to CHDK PTP. At least, most of the big things you question seem to be a consequence of this choice.
Agreed.
Quote
I think the rationale has been given in the past, but it comes down to me having preferred a minimal CHDK PTP. And a large argument for that is that adding more to it makes incompatible protocol revisions more likely. The new functionality is not a part of CHDK PTP, just something that is used through it.
That seems like a marginal distinction. It is effectively part of the CHDK PTP protocol. To actually get the live view data, any client has to accept a data phase from PTP_CHDK_CallFunction, and then understand the results. Realistically, this is one of the main features people will want PTP for, so it makes sense to have the method of doing that be a first class part of the protocol.

As I said in my earlier post, if this could sanely be done entirely in lua, I'd be all for that, but it can't. So I'd rather extend the main protocol in a clear, well defined way instead of throwing stuff in lua and existing functions just to say we didn't change the protocol. If we use new PTP functions, this will still be backward compatible, old clients won't call them.

Here's my thought on an (IMHO) cleaner way to deal with this.

One new PTP command, PTP_CHDK_GetStreamingData (name subject to change... suggestions ?)
- replaces handle_video_transfer
- input parameters are bit flags defining what should be returned, like handle_video_transfer.  The available input params should be enough for pretty much anything we'd want to do, but if you needed more you could have an additional command to select what data is returned by each poll.
- Isn't specific to live view data, that's just one set of things that can be polled. Should also include flags for script status and script messages so a client can just repeatedly poll this one PTP command and get everything it needs in one send. These don't have to be added in the initial implementation though.
- returns all the requested fields with a structure describing their locations. The structure can be very similar to the offset/size handle_video_transfer does now, but rather than implying the meaning of the size/offset fields by their location, they should be matched to the flags that select output.
- The static framebuffer data (from luaCB_get_video_details) just becomes another field that can be selected. Since you no longer need get the function address of handle_video_transfer, you don't need separate function to get this before you start getting video data. You just request this with the first frame, and leave it out later. Not really clear if it really makes sense to separate these values out of the more dynamic ones that have to be sent every frame.
- Compatibility of the returned structures isn't really any different from current implementation, except if we make them chunks that are included in the index structure, we could expand them in a backward compatible way if needed, or define new flags to select different data.

This is more complicated, more code, but IMO would result in a protocol and code we'd be a lot happier living with down the road .

I will try to come up with an implementation of this (edit: if no one else does or convinces me it's a dumb idea) to see if it actually works out like i think, but I'll have to get my head around the C# code or hack live view into chdkptp first.
« Last Edit: 05 / September / 2011, 23:43:11 by reyalp »
Logged
Don't forget what the H stands for.
*

Offline philmoz

  • *****
  • 3450
    • Photos
Re: CHDK PTP interface
« Reply #575 on: 06 / September / 2011, 04:35:26 »
Apart from the change to PTP_CHDK_CallFunction I'm not sure I really see much difference between the two approaches. One puts the code in Lua although this could be refined (see below), the other puts it in PTP - does the distinction really make a difference?

I can understand the concern over overloading the PTP_CHDK_CallFunction function parameters / return so let me propose a slight variation.

- Change the 'luaCB_get_video_details' function to just return the address of 'handle_video_transfer' (as an int). I would also rename the function to something like 'luaCB_get_liveview_handler'.
- Move all the live view data to be transferred into the 'handle_video_transfer' function (using the bit mask to control what info to return). Again I'd probably rename this function.
- Move the 'handle_video_transfer' function to a separate liveview.c / liveview.h file.
- Add a major / minor version number to the live view data structure.

This means:
- no change to the PTP protocol.
- localised protocol for live view.
- can be extended to other data by adding more 'luaCB_get_???_handler' functions and associated handler functions. This keeps functionality separate and also avoids having a monolithic PTP function that all the data transfer blobs get added into. Alternatively, instead of multiple 'luaB_get_???_handler' functions a parameter to a generic function could be used to determine which handler address to return (a return value of 0 means the handler is not implemented).

Phil.

Edit:
Patch for CHDK (1317) attached to illustrate what I'm proposing, plus updated Session.cs from the sample .Net code.
Note the function I've implemented as luaCB_get_data_handler which returns the address of a handler function could be done as a new PTP function instead - I'm not fussed either way. But I think separating out the handler/transfer code from the Lua / PTP code makes sense.

« Last Edit: 06 / September / 2011, 07:37:52 by philmoz »
Logged
CHDK ports:
  sx30is (1.00c, 1.00h, 1.00l, 1.00n & 1.00p)
  g12 (1.00c, 1.00e, 1.00f & 1.00g)
  sx130is (1.01d & 1.01f)
  ixus310hs (1.00a & 1.01a)
  sx40hs (1.00d, 1.00g & 1.00i)
  g1x (1.00e, 1.00f & 1.00g)
  g5x (1.00c, 1.01a, 1.01b)
  g7x2 (1.01a, 1.01b, 1.10b)
*

Offline mweerden

  • ***
  • 194
Re: CHDK PTP interface
« Reply #576 on: 06 / September / 2011, 11:31:23 »
Quote from: reyalp on 05 / September / 2011, 23:19:54
Quote
I think the rationale has been given in the past, but it comes down to me having preferred a minimal CHDK PTP. And a large argument for that is that adding more to it makes incompatible protocol revisions more likely. The new functionality is not a part of CHDK PTP, just something that is used through it.
That seems like a marginal distinction. It is effectively part of the CHDK PTP protocol.
To me that is (almost) like saying HTTP is part of TCP.

But in any case, my point was more related to preventing non-backwards-compatible protocol updates. And that's also why I wouldn't be a fan of your monolithic PTP_CHDK_GetStreamingData. It only takes one of the flags to change in meaning to destroy backwards compatibility.

Quote from: philmoz on 06 / September / 2011, 04:35:26
I can understand the concern over overloading the PTP_CHDK_CallFunction function parameters / return so let me propose a slight variation.
Perhaps I'm missing your point, but I believe your variation still needs the extended PTP_CHDK_CallFunction. I also don't really see what the concern regarding this extension is.

Quote
- Add a major / minor version number to the live view data structure.
Adding a version is probably a good idea. Instead of adding it to structure you could also make it part of the function name. This allows multiple versions to coexist. Or perhaps a combination where backwards-compatible versions are in the structure, and other revisions use a new function. Not yet sure how much sense this makes, though. ;)
Logged
*

Offline philmoz

  • *****
  • 3450
    • Photos
Re: CHDK PTP interface
« Reply #577 on: 06 / September / 2011, 16:12:12 »
Quote from: mweerden on 06 / September / 2011, 11:31:23
Quote from: philmoz on 06 / September / 2011, 04:35:26
I can understand the concern over overloading the PTP_CHDK_CallFunction function parameters / return so let me propose a slight variation.
Perhaps I'm missing your point, but I believe your variation still needs the extended PTP_CHDK_CallFunction.

No, it can use the original version (see the patch I posted) - because the return is a single 'int' value.
Ignore previous comment - I misunderstood how the PTP / live view client interaction was working.
Back to the drawing board.

Quote
Quote
- Add a major / minor version number to the live view data structure.
Adding a version is probably a good idea. Instead of adding it to structure you could also make it part of the function name. This allows multiple versions to coexist. Or perhaps a combination where backwards-compatible versions are in the structure, and other revisions use a new function. Not yet sure how much sense this makes, though. ;)

Not sure how manageable that would be. For example if an updated client connects to an older version of CHDK then it first has to check if the function name exists for the latest version. If it doesn't then it has to check the the next oldest one, and so on until it finds a function (or give up and fail). Would make it hard to to support older versions of CHDK in the client code.

Similar problem going the other way, if an old client connects to a newer version of CHDK the function won't exist (unless you keep all the function names in CHDK which could get messy) - so how would the client tell if it's a newer version or just not there at all.

Phil.
« Last Edit: 06 / September / 2011, 16:47:05 by philmoz »
Logged
CHDK ports:
  sx30is (1.00c, 1.00h, 1.00l, 1.00n & 1.00p)
  g12 (1.00c, 1.00e, 1.00f & 1.00g)
  sx130is (1.01d & 1.01f)
  ixus310hs (1.00a & 1.01a)
  sx40hs (1.00d, 1.00g & 1.00i)
  g1x (1.00e, 1.00f & 1.00g)
  g5x (1.00c, 1.01a, 1.01b)
  g7x2 (1.01a, 1.01b, 1.10b)
*

Offline mweerden

  • ***
  • 194
Re: CHDK PTP interface
« Reply #578 on: 06 / September / 2011, 17:09:25 »
Quote from: philmoz on 06 / September / 2011, 16:12:12
Not sure how manageable that would be. For example if an updated client connects to an older version of CHDK then it first has to check if the function name exists for the latest version. If it doesn't then it has to check the the next oldest one, and so on until it finds a function (or give up and fail). Would make it hard to to support older versions of CHDK in the client code.
That's quite inevitable (but optional) when supporting multiple versions, I think. So the question is whether or not such support is desirable at all or whether it is all too hypothetical anyway.

Quote
Similar problem going the other way, if an old client connects to a newer version of CHDK the function won't exist (unless you keep all the function names in CHDK which could get messy) - so how would the client tell if it's a newer version or just not there at all.
Is such a distinction relevant? It's not like knowing there is a newer version somehow allows it to do more (except perhaps tell the user).
Logged
*

Offline philmoz

  • *****
  • 3450
    • Photos
Re: CHDK PTP interface
« Reply #579 on: 06 / September / 2011, 18:40:19 »
Quote from: mweerden on 06 / September / 2011, 17:09:25
Quote from: philmoz on 06 / September / 2011, 16:12:12
Not sure how manageable that would be. For example if an updated client connects to an older version of CHDK then it first has to check if the function name exists for the latest version. If it doesn't then it has to check the the next oldest one, and so on until it finds a function (or give up and fail). Would make it hard to to support older versions of CHDK in the client code.
That's quite inevitable (but optional) when supporting multiple versions, I think. So the question is whether or not such support is desirable at all or whether it is all too hypothetical anyway.

But making it harder to support older versions won't help encourage people to do it :)

Quote
Quote
Similar problem going the other way, if an old client connects to a newer version of CHDK the function won't exist (unless you keep all the function names in CHDK which could get messy) - so how would the client tell if it's a newer version or just not there at all.
Is such a distinction relevant? It's not like knowing there is a newer version somehow allows it to do more (except perhaps tell the user).

Giving the end user accurate information is important - a generic 'somethings wrong' message is not good. Being able to tell the user that 'you don't have the live view extensions in CHDK', or 'you have and older/newer version you need to update your CHDK/client program' is preferable.

It's also about style.
To me, changing the function name implies that something has changed about the function itself (new signature, different return value etc).
Changing the version in the data tells me that the protocol has changed - which is probably the more likely scenario.

Phil.
Logged
CHDK ports:
  sx30is (1.00c, 1.00h, 1.00l, 1.00n & 1.00p)
  g12 (1.00c, 1.00e, 1.00f & 1.00g)
  sx130is (1.01d & 1.01f)
  ixus310hs (1.00a & 1.01a)
  sx40hs (1.00d, 1.00g & 1.00i)
  g1x (1.00e, 1.00f & 1.00g)
  g5x (1.00c, 1.01a, 1.01b)
  g7x2 (1.01a, 1.01b, 1.10b)
Pages: 1 ... 53 54 55 56 57 [58] 59 60 61 62 63 ... 125   Go Up
« previous next »
 

Related Topics