forked from knolleary/pubsubclient
-
Notifications
You must be signed in to change notification settings - Fork 4
Feature: Implement fstring topics #72
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
Open
hmueller01
wants to merge
37
commits into
master
Choose a base branch
from
implement-fstring-topics
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+388
−28
Open
Changes from 19 commits
Commits
Show all changes
37 commits
Select commit
Hold shift + click to select a range
e569a74
add defines for needed PROGMEM functions
1a6d44b
implemented writeString(const __FlashStringHelper* ...), writeString_P()
d2fa270
Merge branch 'master' into implement-fstring-topics
07efb35
Merge branch 'master' into implement-fstring-topics
hmueller01 64709f1
fix merge failure
hmueller01 b2fb86c
added writeString(const __FlashStringHelper* ...), writeString_P()
hmueller01 c461807
added __FlashStringHelper
hmueller01 ff1997e
Merge branch 'master' into implement-fstring-topics
hmueller01 5019584
Merge branch 'master' into implement-fstring-topics
hmueller01 79bd37d
Merge branch 'master' into implement-fstring-topics
hmueller01 f8374e8
Merge branch 'master' into implement-fstring-topics
hmueller01 07ebf85
do not use prog_uint8_t, deprecated
hmueller01 42d278f
Merge branch 'master' into implement-fstring-topics
hmueller01 92ce07f
Merge branch 'master' into implement-fstring-topics
hmueller01 3de4409
removed writeString(__FlashStringHelper*), added beginPublishImpl(), …
hmueller01 2df1cf2
added F() macro
hmueller01 30d9095
Merge branch 'master' into implement-fstring-topics
hmueller01 1aec205
implemented writeStringImpl() using template, added __FlashStringHelp…
hmueller01 3ba5637
fixed wrong return type of writeStringImpl()
hmueller01 1fdd6dc
shorten source code size for getting string length
hmueller01 541876a
removed template implementation by classic parameter
hmueller01 ee3b134
Merge branch 'master' into implement-fstring-topics
hmueller01 2d7b1b2
added publish(__FlashStringHelper* topic, __FlashStringHelper* payloa…
hmueller01 2fe356c
make all only beginPublishImpl / writeStringImpl calling functions in…
hmueller01 02f55f9
fix compiler error: moved public inline functions to header file
hmueller01 ae7cd3e
fix compiler error: moved public inline functions to header file
hmueller01 5ea9b74
implement write_P(PGM_P string)
hmueller01 443097d
Initial revision
hmueller01 1a3b765
fix arduino-cli compile error
hmueller01 b7fe4c3
implemented bool subscribeImpl(bool progmem, ...) and bool unsubscrib…
hmueller01 f455953
added subscribe examples
hmueller01 4168491
Merge branch 'master' into implement-fstring-topics
hmueller01 54de124
Merge branch 'master' into implement-fstring-topics
hmueller01 f79d377
Merge branch 'master' into implement-fstring-topics
hmueller01 1676861
tag unused parameter
hmueller01 d1d75ed
Merge branch 'master' into implement-fstring-topics
hmueller01 94a83df
use C++ casting
hmueller01 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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.
There is one really big disadvantage of templates...
For each type that can call this function, the compiler will generate a new blob of code.
Now it is contained as a private function, which will keep it somewhat manageble.
For publicly called functions you really should take care or else you might end up with a compiled blob for types like
const char[10],const char[11],const char[...], ... well you get the point :)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.
Hmm, thanks for the hint. I wasn't aware that the compiler creates new code for every type. The goal was to have just one implementation. But wasting flash does not sound good. Maybe I should not use templates and instead just use a parameter:
size_t PubSubClient::writeStringImpl(bool progmem_string, const char* string, size_t pos) {Do you think this is better?
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.
Well the whole idea of templates is indeed to have the same code from a programmer's perspective. But it should be considered as 'inline' code and thus appears potentially multiple times in the binary when a different (template) type is calling it.
N.B. the same does apply to code written in .h files.
If you want to switch to using
const char*then you must make sure whatever is calling this function is handling potential PGM reads.So either wrap it into a
Stringor feed thiswriteStringImplper char.I don't think adding a bool to indicate type is a good thing as you then do the extra implementation in that function and you potentially introduce bugs as you expect the programmer to always have this pair of bool and string to match.
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.
Haven't seen your answer yet ...
The point is, that I need different implementations in case of PROGMEM and normal RAM. In case of ESP8266 / ESP32 I could always use the PROGMEM functions (
strlen_P()and so on). But not for AVR. There RAM and ROM needs to be handled differently. So to make this lib compatible to all kinds of MCUs I need a double implementation or a switch (which is also there in case of the template). And as this is an internal function I think I can deal with the risk. As the template implementation does have no benefit, I will change to the conventional parameter.