use malloc/free for WiFiManagerParameter* array instead of hard coding. #374
Conversation
add alternate constructor to specifiy max parameter count origional default constructor uses old hard codded value
|
WiFiManager(int max_params = 10); and return to one constructor |
|
hmm not sure how I feel about the constructor params |
|
Is there a case where constructor params don't work? Semantically this is no different than multiple constructors and reduces the code complexity. |
|
I dont like passing params via constructors it causes limitation in the future with regards to backward compatibility |
|
I don't think I understand. Are you saying that there should be no constructors that have parameters? |
|
not really, just that variable arguments should not be passed via a constructor, a constructor should be passed data that an object needs to work, not a variable parameter that can also be set via setters etc. It present maintenance problems in the future. For example what happens when we refactor the parameter system, or abstract it out into a separate dependency then we have this legacy constructor definition that cannot be reused, and has to be deprecated or handled. What happens if we need to add more parameters to constructor , what if they are the same datatype ? Its can become a mess. If we were to say allow a config object to preload parameters, then that would be acceptable. For this i would have the constructor call a default param init, then require a explicit init from the user to reallocate the param size etc. yes its a pita for the user, but it is clear what the code is doing, and they are overriding! default values in the class |
… the parameter array each time the size limit is reached. I used the initial size (WIFI_MANAGER_MAX_PARAMS) as the increment when resizing. Existing data is coppied to the new array and the old one is free'd.
|
Ok. I re-worked the change so that it uses the default constructor and resizes the params array each time the size is exceeded. Better? |
|
Ooh thats a great idea, sorry about this ainwas going to merge and fixup but I have been pre occupied |
|
Made the change simpler too. |
|
I assume you have tested this a bit ? |
|
I've been using it in a few projects constantly since the commits. I don't think we should add limits, if the user wants to add 10,000 params why should we stop them? ;-) |
|
Aside from malloc exceptions im sure they will figure it out. Thanks I will merge this then bring it into my dev branch |
WiFiManager.cpp
Outdated
| WiFiManagerParameter** new_params = (WiFiManagerParameter**)malloc(_max_params * sizeof(WiFiManagerParameter*)); | ||
| // copy old data | ||
| memcpy(new_params, _params, _paramsCount * sizeof(WiFiManagerParameter*)); | ||
| free(_params); |
There was a problem hiding this comment.
Doesn't this malloc->memcpy->free lead to fragmented heap?
As i understand adding 30 items means i will have in heap 10 + 20 unusable free space followed by 30 for _params?
Using realloc may reduce chance of fragmented heap.
There was a problem hiding this comment.
Very good point. I'll update the PR
… returns bool indicating the status of adding the parameter where false indicates it failed to increase the size of _params and did not add the parameter.
|
Changed to == maxparams, and changed max params to 5 |
Dynamically allocate the _params array. Existing no arg constructor uses the same default, 10, as it used to. Added a second constructor that allows setting size. Added Destructor that frees allocated _params.