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

Improvements to HostEd #2768

Merged
merged 4 commits into from
Apr 22, 2024
Merged

Conversation

mikee47
Copy link
Contributor

@mikee47 mikee47 commented Apr 20, 2024

This PR proposes some changes to the Hosted classes

Update simpleRPC to master

With clang-tidy #2648 some failures occur in the simpleRPC library. Updating this to current master solves the issues.
I've also put all the code into the simpleRPC namespace which sorts out the conflict with Vector but makes it available for use if required.

Tidy HostTests module

Also decode the anonymous 'packet' blob so we can see what's in it and compare with spec. should we wish to update it.

Use abstract base class for callbacks

Simplifies implementation since usually all callbacks are required.
Compiler ensures all methods have implementations.
clang-tidy gave potential memory leak indication because of Delegates. Not sure if that's real but this fixes it.

@mikee47 mikee47 changed the title Improvements to HostEd [WIP]Improvements to HostEd Apr 20, 2024
@mikee47 mikee47 changed the title [WIP]Improvements to HostEd [WIP] Improvements to HostEd Apr 20, 2024
mikee47 added 3 commits April 20, 2024 22:33
Simplifies implementation since usually all callbacks are required
`clang-tidy` gave potential memory leak indication because of Delegates. Not sure if that's real but this fixes it.
Reduces memory usage; class delegates use heap.
@mikee47 mikee47 force-pushed the feature/hosted-updates branch 2 times, most recently from e206f6a to c733819 Compare April 21, 2024 15:18
@slaff slaff added this to the 5.2.0 milestone Apr 22, 2024
Don't need `host_init`, just wrap `init`
@mikee47 mikee47 changed the title [WIP] Improvements to HostEd Improvements to HostEd Apr 22, 2024
@mikee47 mikee47 requested a review from slaff April 22, 2024 07:46
@slaff
Copy link
Contributor

slaff commented Apr 22, 2024

@mikee47 I am trying to compile the Basic_Blink using the command below and I get compilation error?!

cd $SMING_HOME/../samples/Basic_Blink
make SMING_ARCH=Host ENABLE_HOSTED=tcp HOSTED_SERVER_IP=192.168.4.1

The error is:

static_assert(sizeof(time_t) == 8, "Expecting 64-bit time_t");

Any idea how to work around this issue?

@mikee47
Copy link
Contributor Author

mikee47 commented Apr 22, 2024

... Expecting 64-bit time_t

I'm going to guess your version of GCC is old; I'm with Fedora on GCC 13.2.1 - what version are you using?

@slaff
Copy link
Contributor

slaff commented Apr 22, 2024

what version are you using?

Gcc version 9.4.0. I will check if I can upgrade on my Ubuntu 20.04 LTS ...

@slaff
Copy link
Contributor

slaff commented Apr 22, 2024

Gcc version 10.5.0 is the latest GCC that I can use ... Hm.. have to think about OS upgrade ...

@mikee47
Copy link
Contributor Author

mikee47 commented Apr 22, 2024

Gcc version 10.5.0 is the latest GCC that I can use ... Hm.. have to think about OS upgrade ...

That should be fine #2758

You can just comment out that assert - I put it there deliberately so we'd know about this sort of thing!

@mikee47
Copy link
Contributor Author

mikee47 commented Apr 22, 2024

@slaff You'll doubtless have run into the other issue attempting to build Basic_Blink for Hosted using a tcp connection: DISABLE_NETWORK is hard-coded in component.mk and has to be changed. I'm thinking that's probably just something to bear in mind using Hosted.

@slaff slaff merged commit ea00370 into SmingHub:develop Apr 22, 2024
46 checks passed
@mikee47 mikee47 deleted the feature/hosted-updates branch April 22, 2024 14:14
@slaff slaff mentioned this pull request May 8, 2024
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants