Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Stream::send() #6979
Stream::send() #6979
Changes from 250 commits
c28e50f
b2dfcfa
5235e73
f5cdd10
c3148c4
40b782c
08fe86e
643dd6d
07b52b5
429ba2f
64c2656
6b14a13
32f8871
90daea7
cf892fd
c0f4a16
dfa48c6
e521195
8d3e51f
3345ef1
7cd36a9
a5658fe
92c99f2
474d5a1
314da51
5d4a507
d20e867
630b6b6
3edf028
ac86464
03519da
bc2b59a
72c3237
7db503a
048aecf
58a6257
fd32e65
e8ffe6d
e480ea5
15ab6cf
9dcc444
3b7effe
e84d9f5
beee304
0d6a4af
2036749
5e7c448
afc2494
dd45763
4196458
f7af80b
d69028c
34a6d00
b3c1e27
4071398
0c3a372
820acf0
8e44e02
d44a839
0e29771
df38f89
1c1bd76
2b637ae
4ccc136
5577b32
db62df4
ec3ffe0
fb2c546
99740f8
fee1307
cc3e01c
caa8f2c
cee7631
968e74f
18a842c
0bfc6e9
9387a25
a6f9518
dca0561
fef29c9
fe9c0a1
66ab5bd
957bb36
ef587f5
09fd253
7027de6
0ee1c96
88264aa
f788e52
d005bbf
225b7c6
fac8992
0a540a6
6898f5e
679134c
dc30f67
13e8512
b5ced49
51b6961
3f9d4cc
b80bfff
d62c719
192faab
951737c
ca27dd7
59da9a4
1ce2351
84a3137
dfb39ca
ceaaf18
0e443d0
42ffd6f
feaccaa
580d553
4d2ce85
bf9ad75
cdd72f9
b69bf75
3350923
f0483e1
9b21424
443f5b4
f290fd5
4ba0805
55bb160
ce3c922
bc29fe6
dd947c8
81f2d6f
f85ddbb
1fd80d1
e96f80d
36a0ddc
10e86ad
915647a
dc7021e
a2cb9b1
9021ed8
b52e61b
46808cc
5f80dce
143f2d6
f692c09
ff1ea5b
0437782
3745126
ddb7242
dcb1085
45f7f6c
e47c162
d4f32af
ef998c0
39f6f56
815b82c
d85f50f
112313d
a378aff
f9a5f18
6d84820
9bc8146
a99c418
3699ad3
8e58e97
b24b7ef
ac223a9
0eb0b55
d6c7203
9d90286
270ce0a
f98e167
211c914
c56e533
8268807
d5d2ee5
9eca03c
0901039
68aa236
2e9d1a0
8e350e0
c7fae60
e24e26c
f314946
cde30e4
d95ddf2
44e585f
6b7c344
b31ec10
e326b0a
0f5d283
06f93a0
ca821a9
3a308c2
afd5954
e786a13
5e4342d
ad580e0
8e064be
93c8cc7
30be04c
c94ba0b
31e3c39
da1f96d
531d794
cd3caad
746fcd2
7b35d70
21a50ed
92a6302
41d1868
abca00b
3c1e104
4779433
84d8bc6
592b440
2783c19
da1240a
5ab7415
b70a002
395dae3
9035f51
94e8d60
36a6bce
711bfe8
dae26cb
e1e6fdb
784771c
908f980
b73a9ca
e0084f3
44fb378
3be63d5
e299474
085dbd2
354288b
c223265
756ca72
747a1a9
bfe7ee7
c0a1c38
1188958
bfad4fc
70eb9e8
d32361c
e88d21e
594d77b
b4ca0dd
81290b6
32eb9bf
f0498b5
fd5f282
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this not virtual?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
::read(char*,size_t)
is not in the reference implementation.Making it virtual would add more overhead to the instances.
Having it inline and casting for user is lighter and more efficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these public? I can't think of a use case where they would be used from a sketch.
I suspect that what you really intend is to have some Streams be peekBuffered while others are not, and that's why you implemented this peek api in Stream.
I think that what you really want is to have a StreamPeekBuffer class in between that implements this api.
So, something like this:
Then, unbuffered classes would inherit directly from Stream as they do now, and buffered classes would inherit from StreamPeekBuffered and have these functions available internally. In other words, the user shouldn't need use these or even know about them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider renaming to hasInputTimeout()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK to make them protected.After a second thought, why would the end user be prevented from using it ?
It'd be legal for a sketch to benefit from direct access that a
Stream::
could offer no ?I don't think I want that 😆
Given the way these classes are written, it is preferable to rely on overloading and keep the inheritance line straight. But I agree that in a template world, what you suggests would have been the preferred way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not a good idea to expose everything in a class. The private/protected keywords exist for very good reasons. You should hide implementation details whenever possible, especially in cases where methods are really internal details.
I'll ask in a different way: what would be the purpose of exposing these as public? What would be a sane use case in a sketch? I can think of use cases in objects that inherit from this, but it doesn't make sense to me to leave these open to the end user implementing a sketch.
The line is being kept straight, hierarchy is straight top to bottom. I am just suggesting to implement common functionality in a base class put in between. That is trivial to implement, and it expresses your real intent:
The point here is that Streams that aren't buffered shouldn't be polluted with dummy implementations of these methods that don't apply to them.
3rd party libs that have objects that inherit from Stream won't be using these methods in their standard implementations, so they should inherit directly from Stream anyways. Unless they actually want to have implementations specific to our ESP and our peekbuffer, in which case they should inherit from out StreamPeekBuffer. From an user interface point of view, handling via a Stream base pointer is still ok for both case, as it should be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get it, because
Client
derives fromStream
, someClient
wouldn't implements these extensions while others would.How is an hypothetical
StreamDirect
class supposed to be kept in a straight line ?I think that would add more confusion to the API.
The peekBuffer API is in the public interface to make it accessible by users. By these functions output buffers are directly accessible without copies. Why would we want to hide it ?