Skip to content
This repository was archived by the owner on Jul 8, 2022. It is now read-only.

Implement commands with PipeBlob in/out #378

Merged
merged 21 commits into from
Oct 18, 2017
Merged

Implement commands with PipeBlob in/out #378

merged 21 commits into from
Oct 18, 2017

Conversation

@Ingvord
Copy link
Member

@Ingvord Ingvord commented Jul 18, 2017

Start

@Ingvord
Copy link
Member Author

Ingvord commented Jul 18, 2017 •
edited
Loading

Looks like it is enough to add corresponding methods to DeviceData. No need to change IDL

Ingvord pushed a commit that referenced this pull request Jul 18, 2017
progress #378: increment version
90c1560
Ingvord pushed a commit that referenced this pull request Jul 18, 2017
progress #378: rename falsely named test
899468a
Ingvord pushed a commit that referenced this pull request Jul 18, 2017
progress #378: progress
03aef02
Ingvord pushed a commit that referenced this pull request Jul 19, 2017
progress #378: progress
a1be5aa
Ingvord pushed a commit that referenced this pull request Jul 20, 2017
progress #378: fix segmentation fault
c846001
Ingvord pushed a commit that referenced this pull request Jul 20, 2017
progress #378: fix misc_devproxy
1057dcd
Ingvord
Ingvord commented Jul 20, 2017
View reviewed changes
src/client/devapi_data.cpp
tc_field = tc->member_type(0);
if(tc_field->kind() == tk_string)
{
// The first field in a DevPipeBlob structure is a string (name field)
Copy link
Member Author

@Ingvord Ingvord Jul 20, 2017

Choose a reason for hiding this comment

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

Magic :)

Ingvord pushed a commit that referenced this pull request Jul 22, 2017
progress #378: increment version
b7c06ce
Ingvord pushed a commit that referenced this pull request Jul 22, 2017
progress #378: rename falsely named test
b157f16
Ingvord pushed a commit that referenced this pull request Jul 22, 2017
progress #378: progress
baee43e
Ingvord pushed a commit that referenced this pull request Jul 22, 2017
progress #378: progress
aaac43a
Ingvord pushed a commit that referenced this pull request Jul 22, 2017
progress #378: fix segmentation fault
f921f5a
Ingvord pushed a commit that referenced this pull request Jul 22, 2017
progress #378: fix misc_devproxy
fa87616
Ingvord added a commit that referenced this pull request Jul 25, 2017
@Ingvord
Progress #378 add cmd_query test
5b62458
@tango-controls tango-controls deleted a comment Jul 25, 2017
@tango-controls tango-controls deleted a comment Jul 25, 2017
@tango-controls tango-controls deleted a comment Jul 25, 2017
@tango-controls tango-controls deleted a comment Jul 25, 2017
@tango-controls tango-controls deleted a comment Jul 25, 2017
@tango-controls tango-controls deleted a comment Jul 25, 2017
@tango-controls tango-controls deleted a comment Jul 25, 2017
@tango-controls tango-controls deleted a comment Jul 25, 2017
@tango-controls tango-controls deleted a comment Jul 25, 2017
@tango-controls tango-controls deleted a comment Jul 25, 2017
@tango-controls tango-controls deleted a comment Jul 25, 2017
@tango-controls tango-controls deleted a comment Jul 25, 2017
@tango-controls tango-controls deleted a comment Jul 25, 2017
@tango-controls tango-controls deleted a comment Jul 25, 2017
@tango-controls tango-controls deleted a comment Jul 25, 2017
ingvord and others added 8 commits October 18, 2017 12:54
@Ingvord
progress #378: progress
87b5bed
@Ingvord
progress #378: progress
d0c24cd
@Ingvord
Progress #378 add cmd_query test
593a9eb
@Ingvord
Progress #378 extract/insert
e514b62
@Ingvord
Progress #378 DeviceData operators
09515a9
@Ingvord
Progress #378 codacy
54cbcf4
@bourtemb @Ingvord
More complex DevicePipeBlob test which leads to a crash with the curr…
66526b5 
…ent implementation
@bourtemb @Ingvord
Update DeviceData::insert(DevicePipeBlob *) method to manage memory as
2e62aa5 
it is done for DevEncoded type which is very similar to DevPipeBlob CORBA type.
There is no longer memory ownership transfer when DeviceData::insert is invoked.
There is still no copy of the blob data during the insert in this version.
@Ingvord Ingvord force-pushed the pipe-blob-in-commands branch from 3046ebd to 2e62aa5 Compare October 18, 2017 10:55
@Ingvord
Copy link
Member Author

Ingvord commented Oct 18, 2017

Hi Igor,

As far as I remember, there were actually at least 2 remarks:
It was very difficult to review the PR because there was no easy way to differentiate changes coming from the implementation of this features and changes coming from other commits fixing bugs or implementing other features.
So I advised to do an interactive rebase with only the commits relevant to this PR so we could review this PR in good conditions.
There was a concerned raised by Emmanuel on the potential performance problems with using CORBA::Any for the transfer of DevPipeBlob data.
Kind regards,
Reynald

1 - I do not see any non-relevant commits in the history; maybe this was due to non-rebased state of the branch at the time review was performed. Anyway I am not very strong at git to do interactive rebase as I always mess it
2 - I would postpone performance consideration till the time we have reasonable code base quality so we can observer all other (maybe much more important) issues

Taking 1,2 into account I think it is OK to merge this PR now

@Ingvord Ingvord merged commit 273a04d into master Oct 18, 2017
@bourtemb
Copy link
Member

bourtemb commented Oct 18, 2017

Did you notice that Travis CI is broken because it is trying to clone cppTango-378 branch from tango-idl?
As a minimum, I think we should check Travis CI status before merging.
Sorry for not finding the time yet to review this PR under good conditions after you rebased it correctly.

@Ingvord
Copy link
Member Author

Ingvord commented Oct 18, 2017

@bourtemb this is already fixed in master: https://travis-ci.org/tango-controls/cppTango/builds/289485564

It is now failing due to https://travis-ci.org/tango-controls/cppTango/builds/289511969

cherry-pick of b719402

@bourtemb
Copy link
Member

bourtemb commented Oct 18, 2017

@bourtemb this is already fixed in master: https://travis-ci.org/tango-controls/cppTango/builds/289485564

Good! :-)

It is now failing due to https://travis-ci.org/tango-controls/cppTango/builds/289511969
cherry-pick of b719402

Not good :-(

@Ingvord
Copy link
Member Author

Ingvord commented Oct 18, 2017

Not good :-(

Yeah, I have reverted this commit and will create an issue

Ingvord pushed a commit that referenced this pull request Aug 21, 2018
Backport #378: PipeBlob as command's in/out [0,75h]
9d7f8ec
Ingvord pushed a commit that referenced this pull request Aug 21, 2018
Backport #378: PipeBlob as command's in/out [0,75h]
e79a54d
Ingvord pushed a commit that referenced this pull request Aug 22, 2018
Backport #378: PipeBlob as command's in/out [0,5h]
9880b64
@Ingvord Ingvord mentioned this pull request Aug 23, 2018
Closed
Ingvord pushed a commit that referenced this pull request Aug 28, 2018
@Ingvord
Backport #378: PipeBlob as command's in/out [0,75h]
40ccb01
Ingvord pushed a commit that referenced this pull request Aug 28, 2018
@Ingvord
Backport #378: PipeBlob as command's in/out [0,5h]
0ac0bca
Ingvord pushed a commit that referenced this pull request Aug 31, 2018
@Ingvord
Backport #378: PipeBlob as command's in/out [0,75h]
30d4920
Ingvord pushed a commit that referenced this pull request Aug 31, 2018
@Ingvord
Backport #378: PipeBlob as command's in/out [0,5h]
7203630
Ingvord pushed a commit that referenced this pull request Aug 31, 2018
@Ingvord
Backport #378: PipeBlob as command's in/out [0,75h]
ab1362e
Ingvord pushed a commit that referenced this pull request Aug 31, 2018
@Ingvord
Backport #378: PipeBlob as command's in/out [0,5h]
88f9a2b
@t-b t-b deleted the pipe-blob-in-commands branch October 29, 2019 15:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@taurel taurel Awaiting requested review from taurel

1 more reviewer

@bourtemb bourtemb bourtemb left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@Ingvord Ingvord

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Ingvord @bourtemb