reprappro / RepRapFirmware

OO C++ RepRap Firmware
Other
143 stars 120 forks source link

Add check for missing keys in local storage when loading settings. #75

Closed OlegGirko closed 9 years ago

OlegGirko commented 9 years ago

Some keys expected by updated JavaScript are missing when loading settings from local storage after upgrading firmware to version 1.04. For example, storage.get('temps', 'head1') returns undefined, so applying forEach method to it causes error and prevents further JavaScript initialisation.

This problem manifests as "Connect" button disabled after updating firmware and corresponding files on SD card to version 1.04.

This change adds check for return value of storage.get(...) and replaces it with empty array in all places where forEach method is called on this value.

RRP-support commented 9 years ago

Hi Oleg, thanks for the pull request. I got our programmer to look at it. He replied:

"that issue is only fixed in my new Duet Web Control. And Oleg's fix may work, but I generally tend to avoid forEach in JavaScript when I don't know what kind of data I'm dealing with. Using JQuery's $.each() is a better approach IMHO"

Here is his proposed fix for that settings bug:

function getCookies() { // See if any usable settings were saved var settings = storage.get('settings'); if (settings == undefined || !settings.hasOwnProperty('pollDelay') || !settings.hasOwnProperty('layerHeight') || !settings.hasOwnProperty('halfz') || !settings.hasOwnProperty('noOK')) { storage.set('settings', { pollDelay: 1000, layerHeight: 0.24, halfz: 0, noOK: 0 }); } // Check if the saved temperatures may be used var temps = storage.get('temps'); if (temps == undefined || !temps.hasOwnProperty('bed') || !settings.hasOwnProperty('head')) { storage.set('temps', {'bed' : [120, 65, 0], 'head' : [240, 185, 0]}); } }

I'll let him decide on the best approach. Many thanks for your efforts, though, please feel free to continue contributing!

Ian RepRapPro tech support

OlegGirko commented 9 years ago

The proposed solution has one drawback. If there is temps key in storage, and object stored there contains bed property, but not head1 property (as expected after upgrade), bed property will be overwritten with defaults. Hence, it's better to check for each property individually.

Also, it would make sense to check whether saved properties not just exist, but have correct data type.

The better solution will be something like this:

function getCookies() {
    // See if any usable settings were saved
    var settings = storage.get('settings');
    if (!(settings instanceof Object)) settings = {};
    if (!(settings.pollDelay instanceof Number)) settings.pollDelay = 1000;
    if (!(settings.layerHeight instanceof Number)) settings.layerHeight = 0.24;
    if (!(settings.halfz instanceof Number)) settings.halfz = 0;
    if (!(settings.noOK instanceof Number)) settings.noOK = 0;
    storage.set('settings', settings);
    // Check if the saved temperatures may be used
    var temps = storage.get('temps');
    if (!(temps instanceof Object)) temps = {};
    if (!(temps.bed instanceof Array)) temps.bed = [120, 65, 0];
    if (!(temps.head1 instanceof Array)) temps.head1 = [240, 185, 0];
    if (!(temps.head2 instanceof Array)) temps.head2 = [240, 185, 0];
    if (!(temps.head3 instanceof Array)) temps.head3 = [240, 185, 0];
    storage.set('temps', temps);
}
chrishamm commented 9 years ago

Yes, I agree, that's definitely the best solution. But we don't need a "head3" field for the temperature object, OrmerodWebControl accepts only two heads anyway AFAIK. This will change again when we switch over to my DuetWebControl, but it needs more firmware modifications, so it will probably take a while before we get there.

Thank you again for providing this improved function!

OlegGirko commented 9 years ago

@zombiepantslol: I'm glad that you like my solution. :-)

The reason why head3 is present in my code snippet is because it's present in reprap.js version 1.04 for Ormerod 2.

If you don't mind, I'll prepare another pull request with modified getCookies() function.

OlegGirko commented 9 years ago

OK, new pull request created: #76.

RRP-support commented 9 years ago

Thanks Oleg!

Ian RepRapPro tech support