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.

Sign up for GitHub

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

Jump to bottom

Rewrite Network and UdpClient into a classes #256

Merged
merged 5 commits into from
Jul 14, 2023
Merged

Rewrite Network and UdpClient into a classes #256

merged 5 commits into from
Jul 14, 2023

Conversation

TheDevMinerTV
Copy link
Member

I've done the long needed UDPClient and Network namespace to class rewrite ^^

@TheDevMinerTV TheDevMinerTV mentioned this pull request Jun 30, 2023
Open
@ImUrX
Copy link
Member

ImUrX commented Jun 30, 2023

nice branch naming

@TheDevMinerTV
Copy link
Member Author

wdym, it's the first stage of the feature...

@Eirenliel
Copy link
Member

sorry for the merge conflict, but my PR was first 😛

Eirenliel
Eirenliel requested changes Jul 2, 2023
View reviewed changes
src/network/connection.cpp Outdated Show resolved Hide resolved
src/network/connection.h Outdated Show resolved Hide resolved
src/network/manager.cpp Outdated Show resolved Hide resolved
src/network/manager.h Outdated Show resolved Hide resolved
@TheDevMinerTV
Copy link
Member Author

pain

@TheDevMinerTV TheDevMinerTV force-pushed the impl/feature-retrieval branch 2 times, most recently from b403da5 to 7276ade Compare July 2, 2023 20:58
nekomona
nekomona suggested changes Jul 3, 2023
View reviewed changes
Copy link
Contributor

@nekomona nekomona left a comment

Choose a reason for hiding this comment

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

Probably update define label for the new headers could be more clear

src/network/connection.h Outdated Show resolved Hide resolved
src/network/connection.h Outdated Show resolved Hide resolved
src/network/connection.h Outdated Show resolved Hide resolved
src/network/manager.h Outdated Show resolved Hide resolved
src/network/manager.h Outdated Show resolved Hide resolved
src/network/manager.h Outdated Show resolved Hide resolved
nekomona
nekomona suggested changes Jul 3, 2023
View reviewed changes
Copy link
Contributor

@nekomona nekomona left a comment

Choose a reason for hiding this comment

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

Some bug breaking timeout detection found in the connection.cpp from early return

src/network/connection.cpp Show resolved Hide resolved
src/network/connection.cpp Outdated Show resolved Hide resolved
@TheDevMinerTV TheDevMinerTV force-pushed the impl/feature-retrieval branch 6 times, most recently from 1d2b587 to 931e5d9 Compare July 3, 2023 20:06
nekomona
nekomona suggested changes Jul 4, 2023
View reviewed changes
Copy link
Contributor

@nekomona nekomona left a comment

Choose a reason for hiding this comment

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

Timeout detection still need some patches here. Others are LGTM -w-/

src/network/connection.cpp Outdated Show resolved Hide resolved
src/network/connection.cpp Outdated Show resolved Hide resolved
src/network/connection.cpp Show resolved Hide resolved
Vyolex
Vyolex reviewed Jul 4, 2023
View reviewed changes
.editorconfig Outdated Show resolved Hide resolved
src/main.cpp Outdated Show resolved Hide resolved
src/main.cpp Outdated Show resolved Hide resolved
src/sensors/mpu6050sensor.cpp Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Why do we not need to send callibration report anymore?

Copy link
Member Author

Choose a reason for hiding this comment

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

I asked in Discord, Eiren said I can remove it. It's not being parsed anyways.

TheDevMinerTV
TheDevMinerTV commented Jul 4, 2023
View reviewed changes
src/main.cpp Outdated Show resolved Hide resolved
@TheDevMinerTV TheDevMinerTV requested a review from Vyolex July 4, 2023 15:45
nekomona
nekomona approved these changes Jul 4, 2023
View reviewed changes
Copy link
Contributor

@nekomona nekomona left a comment

Choose a reason for hiding this comment

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

Reconnecting and output during reconnection are fine now, LGTM -w-/

Vyolex
Vyolex approved these changes Jul 4, 2023
View reviewed changes
Copy link
Member

@Vyolex Vyolex left a comment

Choose a reason for hiding this comment

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

Can't test functionality anymore today but the code looks good, if someone else has verified it compiles and works then all good for me too.

@nekomona nekomona mentioned this pull request Jul 12, 2023
Merged
@TheDevMinerTV
Copy link
Member Author

@Eirenliel please re-review.

@Eirenliel Eirenliel merged commit ed74944 into main Jul 14, 2023
@Eirenliel Eirenliel deleted the impl/feature-retrieval branch July 14, 2023 18:21
@TheDevMinerTV TheDevMinerTV restored the impl/feature-retrieval branch July 14, 2023 18:29
@TheButlah TheButlah deleted the impl/feature-retrieval branch July 16, 2023 15:41
@ButterscotchV ButterscotchV mentioned this pull request Oct 15, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Vyolex Vyolex Vyolex approved these changes

@nekomona nekomona nekomona approved these changes

@Eirenliel Eirenliel Awaiting requested review from Eirenliel

@TheButlah TheButlah Awaiting requested review from TheButlah

@ImUrX ImUrX Awaiting requested review from ImUrX

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@TheDevMinerTV @ImUrX @Eirenliel @nekomona @Vyolex