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

Correct Reqest Class memory allocation, Improve _sleep_time read-write to EEPROM #217

Closed
wants to merge 2 commits into from

Conversation

Akubi
Copy link
Contributor

@Akubi Akubi commented Sep 17, 2017

Change « Request Class » memory allocation, issue #214

  • Add buffer « char _value[MAX_PAYLOAD+1] » to keep request data
  • Buffer size is to prevent buffer overrun

Improve _sleep_time read-write to EEPROM, issue #215

  • _sleep_time is a long value and bust be split into bytes in order to be saved to EEPROM
  • Use union structure to achieve this
  • Only 3 bytes are saved to EEPROM

@user2684
Copy link
Contributor

Thanks for the great contribution! I do wonder how we can handle a user migrating to this version since _loadConfig() is called at the beginning and if something is already in the EEPROM the calculated sleep time would be different. I think we have two options: 1) using a different EEPROM slot to save this setting so to avoid conflicts (and optionally migrating an old value if any) 2) adding a warning to the release notes about the lack of compatibility.
I'd go for this latter option, what's your thought?

@Akubi
Copy link
Contributor Author

Akubi commented Sep 17, 2017

My opinion, for the moment:

  • You can commit this modification, but is not correcting the PERSIST functionality
  • Add a warning to the release note to inform that PERSIST is under construction

In any case, I’m not sure someone use it.

I just correct the 2 methods _loadConfig and _saveConfig.
Of course no more compatible.

The PERSIST behaviors must be modified in the future.

  • _sleep_time is a long (4bytes) and only 3 byte are saved
  • Service function [3] [4] [5][29] use getValueInt(), can be dangerous
  • After startup _loadConfig() call, _status is not updated (still AWAKE)
  • If service function [3] [4] [5][29] try to set _sleep_time to 0, nothing is send to EEPROM
  • EEPROM_SLEEP_SAVED is never reset to 0

So the _loadConfig and _saveConfig are correctly codes to store 3 bytes, but I believe the PERSIST function must be revised

@Akubi
Copy link
Contributor Author

Akubi commented Sep 17, 2017

Hi,
Do you prefer I change the PR and remove the issue #215 part ?
That can be part of a different PR.

@user2684
Copy link
Contributor

Sorry for the huge delay. I'd split the two if you don't mind so I can start merging this one straight away.
I've also created this issue #222 so we will not forget to re-think PERSIST entirely.
Thanks again!

@Akubi
Copy link
Contributor Author

Akubi commented Sep 24, 2017

Hi, no problem for the "delay". We all have a lot of activities.
Yes, of course, you can splitt.
Thank

@user2684
Copy link
Contributor

Great. If you don't mind applying the changes directly in your branch I think will be quicker, otherwise I have to submit a PR against your repository, you have to merge and I have to merge back into this one :) Thanks!

@Akubi
Copy link
Contributor Author

Akubi commented Oct 1, 2017

PR closed and replaced by PR #225

@Akubi Akubi closed this Oct 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants