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

Release referenced resources in the destructor for ESP8266SSDP #5607

Merged
merged 10 commits into from Jan 22, 2019

Conversation

mcbp223
Copy link
Contributor

@mcbp223 mcbp223 commented Jan 11, 2019

I need this to resolve a crash when I dynamically turn WiFi off, with the SSDP destructor called before WiFi.disconnect().

@devyte devyte self-requested a review January 12, 2019 00:54
libraries/ESP8266SSDP/ESP8266SSDP.cpp Outdated Show resolved Hide resolved
luc-github pushed a commit to luc-github/ESP32SSDP that referenced this pull request Jan 14, 2019
Add proper end() function
Add `SSDP.setDeviceType("upnp:rootdevice"); ` in sample as it may not be obvious that it is an option to appear in Windows Network Panel
@d-a-v
Copy link
Collaborator

d-a-v commented Jan 16, 2019

Regarding those, I am using path shortening in one of my projects
let me know if you want me to make a PR with this change in debug.h.

Yes !

edit: Well, @earlephilhower would have to comment here too

@mcbp223
Copy link
Contributor Author

mcbp223 commented Jan 17, 2019

@d-a-v Now I like more the solution with relative paths, or anything with gcc support as commented later in the thread, since what I've been using is C++ only, and parameter pack metaprogramming hurts your eyes. But for tests, with the following code I get no more /home occurrences inside the .ino.bin file.

#include "Indexes.h"

#ifdef __cplusplus

template <size_t N>
inline constexpr int lastIndexOfSep(const char (&str)[N], int crt) {
	return crt < 0 ? -1
	               : str[crt] == '/' || str[crt] == '\\'
	                   ? crt
	                   : lastIndexOfSep(str, crt - 1);
}

template <size_t N>
inline constexpr int lastIndexOfSep(const char (&str)[N]) {
	return lastIndexOfSep(str, N - 2); // allow for one character filenames
}

template <size_t N_LONG, int LAST_SEP
        , typename Indexes = typename andrivet::ADVobfuscator::Make_Indexes<N_LONG - (LAST_SEP + 1)>::type>
struct FileNameShortener;

  // Partial specialization, used for indices argument deduction
template<size_t N_LONG, int LAST_SEP, int... I>
struct FileNameShortener<N_LONG, LAST_SEP, andrivet::ADVobfuscator::Indexes<I...>> {
	using storage_type = char[sizeof...(I)];
	storage_type buffer_;

	constexpr FileNameShortener(const char (&str)[N_LONG])
		: buffer_ { str[I + (LAST_SEP + 1)]... } {}
};

#ifndef PROGMEM_G
// per solution in https://github.com/esp8266/Arduino/issues/3369
#define PROGMEM_G __attribute__((section(".irom.text.group")))
#endif

#define __SHORT_FILE__ (__extension__({static constexpr FileNameShortener<sizeof(__FILE__), lastIndexOfSep(__FILE__)> PROGMEM_G sf__(__FILE__); FPSTR(sf__.buffer_);}))
#else // not C++
#define __SHORT_FILE__ (__builtin_strrchr(__FILE__, '/') ? __builtin_strrchr(__FILE__, '/') + 1 : __FILE__)
//#define __SHORT_FILE__ __PRETTY_FUNCTION__
//#define __SHORT_FILE__ __FUNCTION__
#endif

@d-a-v
Copy link
Collaborator

d-a-v commented Jan 18, 2019

and parameter pack metaprogramming hurts your eyes

Show me where I wrote that

I like your snippet. While c++ constexpr functions are elegant, are they really needed regarding the simplicity of the c-only define which is also c++ compatible ?

Now I like more the solution with relative paths

It seems easier to replace __FILE__ by __SHORT_FILE__ than to patch gcc no ?
(and according to @earlephilhower these patches are rather hairy and now unmaintained)

@mcbp223
Copy link
Contributor Author

mcbp223 commented Jan 18, 2019

While c++ constexpr functions are elegant, are they really needed regarding the simplicity of the c-only define which is also c++ compatible

As far as I understand, the name of the game here is to convince the compiler to put in the binary the short file name only, for several reasons - to reduce memory consumption, privacy purposes, be able to produce the same exact binary on different machines with different path arrangements. To cut the filename short you'd have to execute a compile-time processing, and constexpr is designed to require the compiler to do a computation at compile-time, instead of at run-time. Also, without the int... I parameter pack gymnastics I wasn't able to completely remove the full filename from the output, no matter what else I tried. Too bad I can't use this in the C parts of the program though.

In the solution I've linked to, @Eszartek didn't modify gcc, but only a config file in his development chain. You seemed to like his method too, in a comment you've made in the thread.

@d-a-v
Copy link
Collaborator

d-a-v commented Jan 18, 2019

Ah! Of course you are right.
I don't know why I was thinking __builtin_strrchr was called at compile time...

@earlephilhower
Copy link
Collaborator

earlephilhower commented Jan 18, 2019

@d-a-v, It's a 1-line change to convert GCC to only use the filename everywhere __FILE__ is used while leaving debug info untouched as far as I can see.

<removed diff which doesn't seem to actually do anything>

The uncommitted GCC patches allow much more granular transformations, not just eliminations, of multiple paths.

This is a neat discussion but not really related to SSDP. Maybe jump to #4520 if you want to get into it, for everyone's sanity?

@earlephilhower
Copy link
Collaborator

@d-a-v, It's a 1-line change to convert GCC to only use the filename everywhere __FILE__ is used while leaving debug info untouched as far as I can see.
....

After actually trying this patch I can see the lines changed don't actually get called on __FILE__ expansion somehow. Too bad, it looked so simple and clean (famous last words). 😞

@devyte devyte merged commit d7094f2 into esp8266:master Jan 22, 2019
@devyte devyte self-assigned this Jan 22, 2019
@devyte devyte added this to the 2.5.0 milestone Jan 22, 2019
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.

None yet

4 participants