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

[Review & Discuss] Update TxQueue to have a backing AVL Tree (Issues #189 #195) #198

Merged
merged 23 commits into from
Mar 6, 2019
Merged
Changes from 1 commit
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
906a5e4
AVL Tree backed queue
ntakouris Jan 29, 2019
01ad9c6
Adhere to proposed code review changes
ntakouris Jan 29, 2019
722f80b
Update CanIOManager.Transmission tests (fixes from queue changes -- m…
ntakouris Jan 29, 2019
f71bf9e
(have to switch to desktop pc)
ntakouris Jan 29, 2019
188eeb8
Fix some errors
ntakouris Jan 29, 2019
e743bf4
private decls then protected then public
ntakouris Jan 29, 2019
4223aea
Add sanity tests for AvlTree
ntakouris Jan 29, 2019
3d16be6
Add multiple entries per key tests for AvlTree
ntakouris Jan 31, 2019
ec17bb7
Remove remove_always (does not keep rotations)
ntakouris Jan 31, 2019
63b48c7
Update CanIOManager.Transmission test
ntakouris Jan 31, 2019
e0d9324
Add rotation tests for AvlTree
ntakouris Feb 4, 2019
6f02ef3
style and naming changes
ntakouris Feb 13, 2019
36701bc
Remove heap allocs, adhere to suggested changes
ntakouris Feb 20, 2019
1a08155
Fix the unconditional for loop, styling & formatting
ntakouris Feb 22, 2019
bd69b04
Remove auto in avl_tree.cpp
ntakouris Feb 22, 2019
2c12d20
Fix tx_queue.cpp tests
ntakouris Feb 22, 2019
41d92b6
naming
ntakouris Feb 22, 2019
6ebda4a
Drop QoS completely
ntakouris Feb 22, 2019
5b207da
Fix explicit this
ntakouris Feb 23, 2019
d5a21fe
Remove one reduntant static cast, add suggested change on balanceOf
ntakouris Feb 28, 2019
5df8572
Braces revert for canio manager
ntakouris Mar 1, 2019
e61e964
Add OOM test for AvlTree node allocation
ntakouris Mar 1, 2019
4986156
Remove null check on getSize, revert min-height Of
ntakouris Mar 1, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Remove one reduntant static cast, add suggested change on balanceOf
ntakouris committed Feb 28, 2019

Verified

This commit was signed with the committer’s verified signature. The key has expired.
tzemanovic Tomas Zemanovic
commit d5a21fe626f98cf6730196e3dd5244d8032d61ef
2 changes: 1 addition & 1 deletion libuavcan/include/uavcan/transport/can_io.hpp
Original file line number Diff line number Diff line change
@@ -37,7 +37,7 @@ struct UAVCAN_EXPORT CanRxFrame : public CanFrame
#endif
};

struct CanTxQueueEntry // Not required to be packed - fits the block in any case
struct UAVCAN_EXPORT CanTxQueueEntry // Not required to be packed - fits the block in any case
{
MonotonicTime deadline;
const CanFrame frame;
5 changes: 3 additions & 2 deletions libuavcan/include/uavcan/util/avl_tree.hpp
Original file line number Diff line number Diff line change
@@ -81,11 +81,12 @@ class UAVCAN_EXPORT AvlTree : Noncopyable
return 0;
}

return static_cast<int16_t>(heightOf(n->left) - heightOf(n->right));
const int32_t diff = heightOf(n->left) - heightOf(n->right);
return static_cast<int16_t>(std::max(-2, std::min(2, diff)));
Copy link
Contributor

Choose a reason for hiding this comment

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

Pavel wanted to use your maxOf but moved into templates.hpp. We'd also need minOf and an int32 version which suggests a template which then brings type traits to play if we want to prevent inefficient uses which will lead to another round of reviews and diving into the c++ spec...

fuck it.

Let's just go with minOf and maxOf here in this class.

I'm really sorry about the churn on this one line.

Copy link
Author

Choose a reason for hiding this comment

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

Given that this fails for lpc11c24, note: mismatched types 'std::initializer_list<_Tp>' and 'int' return static_cast<int16_t>(std::max(-2, std::min(2, diff)));

Do we keep the last version?

Copy link
Contributor

Choose a reason for hiding this comment

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

it should work when you switch back to your own versions of minOf/maxOf. Its failing because of changes to stl (see: @pavel-kirienko is almost always right)

Copy link
Author

Choose a reason for hiding this comment

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

Yes, but do I need to keep the maxOf & minOf as-is, or move it to the templates.hpp?

Copy link
Contributor

Choose a reason for hiding this comment

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

You can keep them here.

}

static int16_t maxOf(int16_t a, int16_t b) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to write this ourselves: https://en.cppreference.com/w/cpp/algorithm/max

Copy link
Member

Choose a reason for hiding this comment

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

If we care about semi-broken C++ implementations like that of NuttX (unless they fixed it since I last dealt with it), we must be extremely careful introducing new dependencies on STL. This is the reason why we have reinvented a huge wheel in uavcan/util/templates.hpp - none of this stuff one would expect to find in the standard library is available in NuttX. It is also why we can't easily just include algorithm, so I suggest keeping this code here unchanged.

I don't think that we should retain such drastic compatibility policies in bluesky, but this legacy code should remain compatible with older platforms.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay. Shouldn't this go into templates.hpp then? I don't want to have multiple "min/max" functions lying around.

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this go into templates.hpp then?

I guess it should.

return static_cast<int16_t>(a > b ? a : b);
return a > b ? a : b;
}
ntakouris marked this conversation as resolved.
Show resolved Hide resolved

static Node* rotateRight(Node* y) {