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

[pprz] Update protocol parser with additional parser #10

Closed
wants to merge 1 commit into from

Conversation

fvantienen
Copy link
Member

Add an optional extra parser for the bytes.
This is useful for example for intermcu, where also bytes from the original PX4 firmware could come in.

@flixr
Copy link
Member

flixr commented Apr 19, 2016

Hm... this seems like a workaround to me... and inconsistent since that extra parser is only added to the pprz_transport and non of the others.

Maybe we should instead try to add the possibility to attach multiple transports/parsers to one link device.

@fvantienen
Copy link
Member Author

That seems like a good idea, but this fix that I have now seems like a good step towards this.
And since I don't have the time to fix this could we maybe first merge this and fix it later on? Because this fix depends on many more PR's I created / going to create to catch up with master.

@flixr
Copy link
Member

flixr commented Apr 19, 2016

but this fix that I have now seems like a good step towards this.

I actually don't think it's a good step in that direction, it's more a stopgap measure that only fulfills your small problem but makes the others more complicated.

Personally I'm not in favor of merging this as is...

The point of splitting pprzlink out was also that you can easily use it in your own external progs.
For that we can't change it all the time, and if we do it should at least be consistent.

How about instead adding a pprz_check_and_parse_extra with the extra parser arg for the time being? Maybe it doesn't even have to be in pprzlink? (It's use more like a convenience function anyway)

@gautierhattenberger what do you think?

@fvantienen
Copy link
Member Author

I'm ok with the pprz_check_and_parse_extra, but I only added this because of the Pixhawk support which is kinda hacked in Paparazzi and intermcu anyway. But for my PR to cleanup the intermcu to work with the latest pprzlink I needed this to fix the Pixhawk support.

This function could as well be in Paparazzi I'm fine with that either, but I rather want a solution fast. Because I already spend 3 days on rebasing, cleaning and fixing stuff for the latest Paparazzi and pprzlink master, and I don't want to do this again.

@gautierhattenberger
Copy link
Member

I completely understand your issue, but I don't like either this modification to pprzlink. I agree with @flixr idea. Since the parse_and_check function is not very long and is more or less a convenient function, I suggest that you just do your own check on the paparazzi side and call the two parsers there.
It leaves us more time to find a cleaner solution to support multiple parsers for all transport.
Is that acceptable for you ?

@fvantienen
Copy link
Member Author

Yes that seems fine then I will close this one ;)

@fvantienen fvantienen closed this Apr 20, 2016
@fvantienen fvantienen deleted the secondary_pprz_parser branch April 20, 2016 06:35
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

3 participants