CHDK PTP interface

reyalp

13878

Re: CHDK PTP interface

« Reply #1170 on: 21 / April / 2019, 17:05:10 »

Advertisements

Thanks for the reports.

Some more random notes:

On A540 (vxworks) PS.FIR is not listed as an object. GetStorageIDs returns 2 items, with volume labels A and V. V is empty and has filesystem type 0x8001 (vendor extension)

The D10 firmware code related to SendObject also contains references to V/... (e.g. sub_FF8D1BA8) but only returns one storage, but only returns one storage, with a null volume label.

code snippets
list storage

Code: [Select]

!info={} for i,id in ipairs(con:ptp_get_storage_ids()) do info[i]=con:ptp_get_storage_info(id) end return info

list objects on all storage

Code: [Select]

!handles=con:ptp_get_object_handles()
!info={} for i,h in ipairs(handles) do info[i]=con:ptp_get_object_info(h) end return info

The OpObjHdl.c Line 2645 assert happens in the SendObject handler, related to the return value of sub_FF9F6458 (in ObjMethdRW.c). This function appears to reference the extension .RPC

The gihidra "Version Tracking" feature might help untangle the differences between R31 and later, but I still haven't dug into it.

I've been thinking about adding the various PTP handlers, send/recv etc functions to the sig finder csv for easier analysis.

edit:
The SendObjectInfo and SendObject handlers are also used for Canon extension opcodes 0x9024 and 0x9025, respectively (table starting at FFAE1924 on d10). Neither appears to check the opcode parameter in R2, but it's also probably in some of the data structures.

« Last Edit: 22 / April / 2019, 00:28:24 by reyalp »

Logged

Don't forget what the H stands for.

reyalp

13878

Re: CHDK PTP interface

« Reply #1171 on: 22 / April / 2019, 01:29:20 »

OK, I figured out the assert on D10. It wants a full path like A/TEST.DAT

(edit: This is somewhat odd since according to the PTP spec, one would set the directory with parent handle, and the returned objects have no path in the filename)

It does not appear to suffer from 512 multiple transfer problem.

« Last Edit: 22 / April / 2019, 01:31:18 by reyalp »

Logged

Don't forget what the H stands for.

reyalp

13878

Re: CHDK PTP interface

« Reply #1172 on: 23 / April / 2019, 01:10:19 »

Using uncached memory for the receiving buffer appears to fix the issue. This can be handled in recv_ptp_data by fiddling the cache bit, but we need a data cache flush function.

Making all the callers umalloc seems like a worse option.

Other notes:
On newer cameras, using A/ on SendObject crashes, so you have to know which form a camera wants before using it.

SendObject has some logic to do buffering with multiple reads, but it appears to have a pretty large buffer so it only used one in the problem case. It's possible this buffer is dedicated and could be re-used by our PTP code without taking away regular RAM, but I didn't track it down. d10 sub_FF8C9374 seems to be connected to the read (and presumably, buffer) size.

Logged

Don't forget what the H stands for.

srsa_4c

4451

Re: CHDK PTP interface

« Reply #1173 on: 23 / April / 2019, 13:24:58 »

Quote from: reyalp on 23 / April / 2019, 01:10:19

SendObject has some logic to do buffering with multiple reads, but it appears to have a pretty large buffer so it only used one in the problem case. It's possible this buffer is dedicated and could be re-used by our PTP code without taking away regular RAM, but I didn't track it down. d10 sub_FF8C9374 seems to be connected to the read (and presumably, buffer) size.

The first routine that has a ComMemMan.c assert allocates 1MB uncached exmem (type 1, probably EXMEM_COM). I'd guess the buffer is there.
I wonder if that allocation can clash with those video buffers that need to be skipped when CHDK is loaded in exmem - most new-ish models don't support native remote shooting (except for the newest models that do).

Nice investigation, btw.

Logged

reyalp

13878

Re: CHDK PTP interface

« Reply #1174 on: 23 / April / 2019, 16:54:07 »

Quote from: srsa_4c on 23 / April / 2019, 13:24:58

The first routine that has a ComMemMan.c assert allocates 1MB uncached exmem (type 1, probably EXMEM_COM). I'd guess the buffer is there.

Thanks for that.

Quote

I wonder if that allocation can clash with those video buffers that need to be skipped when CHDK is loaded in exmem - most new-ish models don't support native remote shooting (except for the newest models that do).

Good point. Seems likely since we weren't able to find a way to avoid those when allocating exmem. I guess if CHDK is already using EXMEM, the allocation for EXMEM_COM should fall outside it, but non-exmem enabled cams could see corruption.

Logged

Don't forget what the H stands for.

philmoz

3445

Re: CHDK PTP interface

« Reply #1175 on: 24 / April / 2019, 20:22:08 »

Small patch for chdkptp to fix a crash on MacOS with libusb.
Also fixes a compiler warning in properties.c

chdkptp_macos.patch (1.78 kB - downloaded 11 times.)

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)

reyalp

13878

Re: CHDK PTP interface

« Reply #1176 on: 24 / April / 2019, 23:44:20 »

Quote from: philmoz on 24 / April / 2019, 20:22:08

Small patch for chdkptp to fix a crash on MacOS with libusb.
Also fixes a compiler warning in properties.c

Thanks. Added in r859

Logged

Don't forget what the H stands for.

reyalp

13878

Re: CHDK PTP interface

« Reply #1177 on: 25 / April / 2019, 01:28:21 »

Quote from: reyalp on 23 / April / 2019, 01:10:19

Using uncached memory for the receiving buffer appears to fix the issue. This can be handled in recv_ptp_data by fiddling the cache bit, but we need a data cache flush function.

This turns out to be more annoying than I had hoped. The cached address passed to recv_ptp_data isn't necessarily aligned to a cache line, so simply flushing or cleaning and flushing can corrupt neighboring data.

The attached patch cleans before the transfer (ensuring everything is written out from cache to RAM before the transfer starts) and cleans+flushes after, ensuring that subsequent reads will see the data that was DMAd to RAM. It's not applied to digic 6, but digic 6 cams don't appear to have the original problem.

This appears to "work" based on intensive msgtest testing, but AFAIK is still incorrect, because other tasks could write to cache in the in between. E.g. if malloc related structures happen to share a cache line with buffer, and another task malloc's something, it could get hosed. Stack buffers could also be problematic, since context switches put stuff on the tasks stack.

AllocateUncacheableMemory pads allocations with an extra 64 bytes, likely to avoid this kind of thing.

The patch doesn't seem to have any measurable performance impact.

tl;dr
It's probably better to use dedicated buffers or umalloc. This is kind of unfortunate, since transfers of files or large scripts can use a significant fraction of free memory. umalloc may also have less memory available, since it only allocates from the Canon heap.

It's also annoying that this only appears to be need for a small subset of camera, and only at specific transfer size. I guess we could use a small uncached buffer only on those cameras when needed, but that seems pretty hokey. ptp.c is also current platform independent code.

edit 2019 04 25:
Patch updated to fix digic 6 ifdef.

ptp-length-bug-fix-work-4.patch (2.74 kB - downloaded 10 times.)

« Last Edit: 25 / April / 2019, 22:20:21 by reyalp »

Logged

Don't forget what the H stands for.

srsa_4c

4451

Re: CHDK PTP interface

« Reply #1178 on: 25 / April / 2019, 18:53:24 »

Quote from: reyalp on 25 / April / 2019, 01:28:21

tl;dr
It's probably better to use dedicated buffers or umalloc. This is kind of unfortunate, since transfers of files or large scripts can use a significant fraction of free memory. umalloc may also have less memory available, since it only allocates from the Canon heap.

It's also annoying that this only appears to be need for a small subset of camera, and only at specific transfer size. I guess we could use a small uncached buffer only on those cameras when needed, but that seems pretty hokey. ptp.c is also current platform independent code.

We could also make a umalloc implementation that could use all memory available to CHDK, and use that on cameras thought to be affected (replacing mallocs in ptp code, if that's even possible). The change you made in recv_ptp_data() is quite large (took out that while block), but I can't say I'm familiar with that code. One of the ifdefs has a typo.

I took a look at the ComMemMan routines. The first(?) two inits and uninits the communication memory, the following two look like an allocator and a de-allocator. But, as previously mentioned, we don't necessarily want to use memory from there due to corruption risk.

Logged

reyalp

13878

Re: CHDK PTP interface

« Reply #1179 on: 25 / April / 2019, 22:30:16 »

Quote from: srsa_4c on 25 / April / 2019, 18:53:24

The change you made in recv_ptp_data() is quite large (took out that while block), but I can't say I'm familiar with that code. One of the ifdefs has a typo.

Yeah, that loop was the "buffering" logic I mentioned earlier. It was a leftover that didn't really make sense: in practice size was almost always <= buffer_size (half of the largest free block at the start of the PTP operation), and there appears to be no reason to operate in smaller chunks.

Thanks for catching the typo, I hadn't got around to testing it on digic 6. Patch updated.

I've now tested on a540, d10, sx160, and elph180, sx710, g7x like

Code: [Select]

!m=require'extras/msgtest'
!m.test{size=1,sizeinc=1,count=10000,gc='step'}
!m.test{size=500,sizeinc=128,count=400,gc='step'}

The first tests single byte increments. The second covers a larger range including the problem (0x23f4 +n*512) values. It might fail on low memory cams trying to send a ~50k message.

Quote

I took a look at the ComMemMan routines. The first(?) two inits and uninits the communication memory, the following two look like an allocator and a de-allocator. But, as previously mentioned, we don't necessarily want to use memory from there due to corruption risk.

Agreed. I guess we could maybe use a similar approach to CHDK exmem and allocate an unused amount to cover the video buffers (if it allocates in a predictable way), or only use it in playback, but neither seems attractive for the amount of work.

Quote

We could also make a umalloc implementation that could use all memory available to CHDK, and use that on cameras thought to be affected (replacing mallocs in ptp code, if that's even possible).

It's not that bad, but in the case of script messages, the memory is passed on and freed elsewhere. When I was testing, I made a version of memmgmt.c free() that detected uncached memory and called ufree on it. Any processing on the allocated buffer would be slow, but it's likely insignificant compared to the transfer. Script is compiled directly from the buffer. Messages are copied into Lua strings.

uses of recv_ptp_data

flush_recv_ptp_data
mallocs/frees buffer. Discards results.

PTP_CHDK_SetMemory
Target address passed direct. Not implemented in chdkptp

PTP_CHDK_CallFunction
mallocs and frees a typically very small buffer for function arguments

PTP_CHDK_TempData
mallocs buffer for filename used with subsequent camera->pc transfer. Freed by additional TempData operation

PTP_CHDK_UploadFile
mallocs up to half largest free block, transfers in chunks, frees when complete. Currently uses fopen, but could be switched to open/write which also want uncached memory.

PTP_CHDK_ExecuteScript
mallocs and frees buffer with script source. Lua is compiled from the buffer, so performance impact could be relatively significant. Potentially large.

PTP_CHDK_WriteScriptMsg
mallocs message, which includes header and content. Freed by lua when de-queued. Potentially large

Logged

Don't forget what the H stands for.