Conversation
remove no-scp and mwl option
update readme
Enet4
left a comment
There was a problem hiding this comment.
Thank you very much for the initiative! With the little tests I did so far this looks quite promising.
I left some suggestions inline. Consider also rebasing the branch to clear the errors reported in CI.
| @@ -0,0 +1,153 @@ | |||
| //! Module for parsing query text pieces into DICOM queries. | |||
There was a problem hiding this comment.
We're starting to see some repetition, but this can be improved later.
Co-authored-by: Eduardo Pinho <enet4mikeenet@gmail.com>
Co-authored-by: Eduardo Pinho <enet4mikeenet@gmail.com>
Co-authored-by: Eduardo Pinho <enet4mikeenet@gmail.com>
Co-authored-by: Eduardo Pinho <enet4mikeenet@gmail.com>
Co-authored-by: Eduardo Pinho <enet4mikeenet@gmail.com>
Co-authored-by: Eduardo Pinho <enet4mikeenet@gmail.com>
knopkem
left a comment
There was a problem hiding this comment.
Any chance that this gets merged?
Enet4
left a comment
There was a problem hiding this comment.
Hello @knopkem! I was close to approving this pull request two weeks ago, but I encountered some oddities that called for further investigation. Can you please see the comments inline, especially the one about matches? Feel free to ask any follow-up questions.
| }; | ||
|
|
||
| println!( | ||
| "------------------------ Match #{i} ------------------------" |
There was a problem hiding this comment.
I wonder what constitutes a match here.
Per PS3.9 section 9.1.4.2, the responses from the Move SCP are not exactly "matches" but rather information about the situation with the move, so what we usually receive are "status pending" messages and a final response before closing. And some implementations only do the latter without waiting for the transfer to occur, so I've had this tool telling me that there were no matches, even though the request was processed successfully.
This match printing logic is probably a remnant of C-FIND, and it should be translated to the C-MOVE use case. My suggestion would be showing a progress indicator that ticks every time a status pending response emerges, and stopping depending on the response status.
There was a problem hiding this comment.
Yes, this is a plain copy from the c-find tool and does not really apply here, although it's only for user output and not functionality. Anyway, will try to get a basic progress running.
There was a problem hiding this comment.
Seeing that this is the only blocker, what we can do here now is:
- remove attempts at reading PData C-MOVE-Rsp IODs (since AFAIK C-MOVE responses only return DIMSE commands to the C-MOVE SCU)
- dump command objects in verbose mode, calling them a status report rather than a match
- change some of the logs to make more sense
There was a problem hiding this comment.
Hey, sorry that there was no progress on this, but didn't find much time for it recently and also I'm not very proficient with rust (actually I wanted to base something on your lib to learn rust). So yes, would be good to get this merged but not sure when I find time for it, so maybe it would be better if you polish the rest of it. Thanks
Co-authored-by: Eduardo Pinho <enet4mikeenet@gmail.com>
Enet4
left a comment
There was a problem hiding this comment.
Would you be able to pick this up again, or would you prefer that I take over?
| }; | ||
|
|
||
| println!( | ||
| "------------------------ Match #{i} ------------------------" |
There was a problem hiding this comment.
Seeing that this is the only blocker, what we can do here now is:
- remove attempts at reading PData C-MOVE-Rsp IODs (since AFAIK C-MOVE responses only return DIMSE commands to the C-MOVE SCU)
- dump command objects in verbose mode, calling them a status report rather than a match
- change some of the logs to make more sense
Co-authored-by: Eduardo Pinho <enet4mikeenet@gmail.com>
Co-authored-by: Eduardo Pinho <enet4mikeenet@gmail.com>
|
Hmm, this pull request is tied to your |
Based on the existing c-find and c-store tools. Rust beginner here, don't hesitate to correct things.