Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement slicing #47

Merged
merged 6 commits into from
Apr 26, 2017
Merged

Implement slicing #47

merged 6 commits into from
Apr 26, 2017

Conversation

hunt0r
Copy link
Collaborator

@hunt0r hunt0r commented Apr 16, 2017

This is a first attempt at slicing for LogMsgGroups as well as Ardupilogs. (NOTE: If we accept the re-organization from PR #46 , this one merges well with that too)

I made a number of initial decisions we could reconsider... such as:

  1. what to do if an invalid slice is requested
  2. what to do if there's no data inside the slice endpoints

No doubt as we do more slicing and better understand our needs, we might re-factor or even re-define how this works.

Copy link
Owner

@Georacer Georacer left a comment

Choose a reason for hiding this comment

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

In all, it seems to work as intented. Please address my questions.
I don't really like the logMsgGroups property solution for tracking the different messages, so I may add some patches, after we talk.

slice.numMsgs = 0;

% Loop through the LogMsgGroups, slicing each one
for msgGroup = slice.logMsgGroups
Copy link
Owner

Choose a reason for hiding this comment

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

I don't understand the utility of the new logMsgGroups property. It seems to hold all the discovered message types as sub-properties, but apart from that I don't see anything else.
Especially since all the actual LogMsgGroups objects are properties of the Ardupilot object, not the Ardupilot.logMsgGroups property.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For copying, I think we need some way to loop through all the LogMsgGroups in an Ardupilog and copy each.

One way to do it is keep the meta.DynamicProperty objects created during the "addprop()" call.

Another way I can think of, which I think you utilized in #50 is to loop through all properties and query each with isa(property, 'LogMsgGroup') before proceeding.

Do you prefer the 2nd way, or perhaps another way I haven't written here? I don't have a preference.

Copy link
Owner

Choose a reason for hiding this comment

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

I'd prefer to avoid creating another global property when a local approach is possible.
But I don't want to make you jump through hoops.
I'll merge this now and add an issue for me to change this eventually with the 'isa()' approach you suggested.


end
methods(Access=protected)
function cpObj = copyElement(obj)
Copy link
Owner

Choose a reason for hiding this comment

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

Matlab does, in general, a poor job of copying objects. Maybe you will like this as a better implementation of a deep copy.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It looks very clean. Where do the component functions come from?

Would you prefer to use this implementation instead? I don't have a preference

Copy link
Owner

Choose a reason for hiding this comment

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

I think they're native. I don't have them installed separately.
Sorry for merging this too soon. We'll make the changes later.

@Georacer Georacer merged commit 4ca9001 into master Apr 26, 2017
@Georacer Georacer deleted the implement_slicing branch April 26, 2017 15:33
@hunt0r hunt0r mentioned this pull request Apr 26, 2017
7 tasks
@Georacer Georacer mentioned this pull request Apr 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants