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

event args constructors made public to allow external code to call it #10

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

valeriob
Copy link
Contributor

No description provided.

@jefffhaynes
Copy link
Owner

Generally speaking I think this is considered bad practice. Can you explain why something external to the library need to create events?

@valeriob
Copy link
Contributor Author

running the library with debugger attached spins up 1 cpu at 100% load and slows packets down, so i'm using a custom deserializer, but i would like to keep using most of the library, like packages definition and events to be able to swap it without code change.

@jefffhaynes
Copy link
Owner

jefffhaynes commented Feb 25, 2018 via email

@valeriob
Copy link
Contributor Author

Agree, but I don't have time to diagnose the serialization library right now.
Let me use a custom one until we can diagnose the problem, I don't see any issue on exposing those constructors

@jefffhaynes
Copy link
Owner

jefffhaynes commented Feb 25, 2018 via email

@valeriob
Copy link
Contributor Author

Just reading from a ublox device that outputs 3 message types with very low frequency, I'll prepare a repro as soon as I can. For now can you just help me be compatible ?

@jefffhaynes
Copy link
Owner

jefffhaynes commented Feb 25, 2018 via email

@valeriob
Copy link
Contributor Author

Have you run the test with debugger attached ?

@jefffhaynes
Copy link
Owner

jefffhaynes commented Feb 25, 2018 via email

@valeriob
Copy link
Contributor Author

valeriob commented Feb 26, 2018

I've prepared a repro:
checkout this commit https://github.com/valeriob/ublox/tree/600f74cedbd79176dcb207cf444697c0ab4da0d6
Run TestClient on your com port, Uncomment RunWithBinarySerializer(device); for the standard version or RunWithManualSerializer(serial, source); for the manual serialization.

My device sends 👍 UBX NAV-PVT, UBX NAV-SVINFO, UBX NAV-STATUS, UBX NAV-RELPOSNED.
In the first case you will see your first core taking 100% load, in the second case, 1%.

@valeriob
Copy link
Contributor Author

hi @jefffhaynes could you check on it ?
can you please make those constructor public so we can reuse all the work of the spec ?
thanks

@jefffhaynes
Copy link
Owner

jefffhaynes commented Mar 19, 2018 via email

@valeriob
Copy link
Contributor Author

I understand what you mean @jefffhaynes , but that's not a work around, it's a feature.
this will allow to use custom serialization allowing to use the same API and messages definition. It would be very usefull anyway.

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.

2 participants