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

Upgrade to the latest master (2.0-pre, then 2.0) #1

Closed
pfalcon opened this issue Aug 15, 2019 · 7 comments
Closed

Upgrade to the latest master (2.0-pre, then 2.0) #1

pfalcon opened this issue Aug 15, 2019 · 7 comments
Labels
enhancement New feature or request

Comments

@pfalcon
Copy link

pfalcon commented Aug 15, 2019

As github says for this repo:

This branch is 13 commits ahead, 1550 commits behind zephyrproject-rtos:master.

I.e., there's quite a lot of developments went into Zephyr lately, and it makes sense to work on top of them, to make sure GoogleIoT sample works with soon-to-be-released Zephyr 2.0 (and of course, the sample would go into master, so we should test with it).

Proposed plan: I'll test the rebase, and if works as expected, will ask @atigyi to perform the rebase on his side and push it. Let me know how that sounds.
@pfalcon pfalcon added the enhancement New feature or request label Aug 15, 2019
@atigyi
Copy link
Owner

atigyi commented Aug 15, 2019

sounds good!

@pfalcon
Copy link
Author

pfalcon commented Aug 16, 2019

@atigyi, Thanks for acking. As a quick update, I tried to rebase, but found that due to a recent DNS optimization, a case like used by GoogleIoT regressed. We're looking into it.

@pfalcon
Copy link
Author

pfalcon commented Aug 20, 2019

Ok, so the DNS bug is fixed and pushed to master. I've performed a rebase against Zephyr revision bd72ea1 (zephyrproject-rtos@bd72ea1). There were a couple of trivial conflicts in west.yaml, easy to resolve. For reference, I pushed rebased version in my repo, https://github.com/pfalcon/zephyr/tree/google_iot_device_sdk_integration-pfalcon

With that, I confirm that the google_iot_device sample works for me as expected. @atigyi, can you please perform such a rebase and force-push here? Thanks.

This was referenced Aug 21, 2019
Closed
Open
@atigyi
Copy link
Owner

atigyi commented Aug 26, 2019 •
edited
Loading

@pfalcon, Good news is that the google_device_sdk_integration branch is at tag v2.0.0-rc1 and the google_iot_device sample works on linux. Bad news is I used git merge instead of your advice rebase. This means the git history got bloated. Sorry for that. What to do with this? git-squash all the commits applied since the readme update? Or revert + rebase?

@pfalcon
Copy link
Author

pfalcon commented Aug 27, 2019

Bad news is I used git merge instead of your advice rebase.

@atigyi, thanks for giving it a try! But yeah, that's indeed not too bright news ;-). The point, our work should follow the direction of: on each step, resolve issue and/or reduce technical debt, to lead the changes into a shape which can be submitted as PR to Zephyr upstream. But Zephyr doesn't accept PRs with merge commits (it follows "clean linear history" model, which means rebase workflow model for any WIP changes). So, if we add git merges, we're just accumulating technical debt. Once we got stuff running on qemu_x86, instead of shooting a PR upstream, we'll say "OMG, those merge commits, we yet need to deal with them". So yeah, it would be nice to just avoid them now.

Or revert + rebase?

That's what I'd suggest. There's another option - I did this rebase (well, would need to redo on the latest master, there's another important fix went in). So, if it would work for you to take much branch and push into yours, that's another option. But generally, the idea is that you guys take care of this branch, and at all times know and aware how it evolves. So, if I can help anyhow to setup this rebasing process, please let me know. For example, if it would be useful if just recorded all my action during rebase process, as a kind of walk-thru, I can do that.

Thanks!

atigyi pushed a commit that referenced this issue Aug 29, 2019
@nashif
mempool: don't over-size the free block bitmap
744df1e 
Currently, the free block bitmap is roughly 4 times larger than it
needs to, wasting memory.

Let's assume maxsz = 128, minsz = 8 and n_max = 40.

Z_MPOOL_LVLS(128, 8) returns 3. The block size for level #0 is 128,
the block size for level #1 is 128/4 = 32, and the block size for
level #2 is 32/4 = 8. Hence levels 0, 1, and 2 for a total of 3 levels.
So far so good.

Now let's look at Z_MPOOL_LBIT_WORDS(). We get:

Z_MPOOL_LBIT_WORDS_UNCLAMPED(40, 0) = ((40 << 0) + 31) / 32 = 2
Z_MPOOL_LBIT_WORDS_UNCLAMPED(40, 1) = ((40 << 2) + 31) / 32 = 5
Z_MPOOL_LBIT_WORDS_UNCLAMPED(40, 2) = ((40 << 4) + 31) / 32 = 20

None of those are < 2 so Z_MPOOL_LBIT_WORDS() takes the results from
Z_MPOOL_LBIT_WORDS_UNCLAMPED().

Finally, let's look at _MPOOL_BITS_SIZE(. It sums all possible levels
with Z_MPOOL_LBIT_BYTES() which is:

  #define Z_MPOOL_LBIT_BYTES(maxsz, minsz, l, n_max)    \
        (Z_MPOOL_LVLS((maxsz), (minsz)) >= (l) ?        \
         4 * Z_MPOOL_LBIT_WORDS((n_max), l) : 0)

Or given what we already have:

Z_MPOOL_LBIT_BYTES(128, 8, 0, 40) = (3 >= 0) ? 4 * 2  : 0 = 8
Z_MPOOL_LBIT_BYTES(128, 8, 1, 40) = (3 >= 1) ? 4 * 5  : 0 = 20
Z_MPOOL_LBIT_BYTES(128, 8, 2, 40) = (3 >= 2) ? 4 * 20 : 0 = 80
Z_MPOOL_LBIT_BYTES(128, 8, 3, 40) = (3 >= 3) ? 4 * ??

Wait... we're missing this one:

Z_MPOOL_LBIT_WORDS_UNCLAMPED(40, 3) = ((40 << 6) + 31) / 32 = 80

then:

Z_MPOOL_LBIT_BYTES(128, 8, 3, 40) = (3 >= 3) ? 4 * 80 : 0 = 320

Further levels yeld (3 >= 4), (3 >= 5), etc. so they're all false and
produce 0.

So this means that we're statically allocating 428 bytes to the bitmap
when clearly only the first 3 Z_MPOOL_LBIT_BYTES() results for the
corresponding 3 levels that we have should be summed e.g. only
108 bytes.

Here the code logic gets confused between level numbers and the number
levels, hence the extra allocation which happens to be exponential.

Signed-off-by: Nicolas Pitre <npitre@baylibre.com>
@atigyi
Copy link
Owner

atigyi commented Aug 29, 2019 •
edited
Loading

Ok, seems to be fixed, see commit history. Base is at the 33000th commit of Zephyr/master eddf058 :)
Thanks for your help!
Also tried the example, connects, publishes to Cloud IoT!

@pfalcon
Copy link
Author

pfalcon commented Aug 29, 2019

@atigyi, Great work, thanks! Confirming that the latest google_iot_device_sdk_integration branch, rev f9abdbc works as expected for me with BOARD=native_posix.

I hope we'll handle further rebases on regular basis to keep delta with the upstream small, so closing this ticket.

@pfalcon pfalcon closed this as completed Aug 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@pfalcon @atigyi