CANopenNode / CANopenEditor

CANopen Object Dictionary Editor
GNU General Public License v3.0
141 stars 64 forks source link

Set defaut value to all subs with empty default values #142

Closed trojanobelix closed 2 weeks ago

trojanobelix commented 3 weeks ago

I find it annoying to set the default value individually for many subs. Often they should all be set to the same value.

The PR sets all empty subs that have no default value to the one just entered. Can then be changed at any time. Better a wrong one than none.

Not setting a value to all subs leads to warnings when saving (I assume that DS301 does not allow this, but I am unsure).

Comments wellcome.

CANopenNode commented 3 weeks ago

Sometimes it is intended to keep default value unset. For example here: https://github.com/CANopenNode/CANopenDemo/blob/1f16f496cf9bec459d0c86307572eac7cdbc2ebd/demo/demoDevice.eds#L2762-L2769

I didn't notice problems with unset default value. CANopenNode V4 preserves memory, if default value is not set (value is calculated internally in such case), especially if there are many long arrays.

I think, user should have an option to have default value unset. I added a new commit to this PR, which includes some checks and MessageBox to ask user.

CANopenNode commented 3 weeks ago

Please check if it works now. I edited online on github.

nimrof commented 3 weeks ago

Not testet, just a quick comment/question.

I might be wrong here, but that dialog would appear even if all the subs have default value?

If i am correct so would it not be better to check if they to not have default value and if they don't, then show the dialog?

CANopenNode commented 3 weeks ago

I might be wrong here, but that dialog would appear even if all the subs have default value?

If i am correct so would it not be better to check if they to not have default value and if they don't, then show the dialog?

I was thinking about that, but I was a little lazy. Extra loop would be required.

I such a (rare) case, answering yes or no would make no difference.

CANopenNode commented 3 weeks ago

I just found generated binaries for checks in pull request. For binaries choose details for one of the checks, then choose summary. At the bottom of the page are Artifacts, where binaries can be downloaded.

trojanobelix commented 3 weeks ago

Does anyone know why particulate default values that are not set trigger a warning when saving the project file? EDSChk from Vector has no problem with this (EDS, XDD1.0 checked)

trojanobelix commented 3 weeks ago

I just found generated binaries for checks in pull request. For binaries choose details for one of the checks, then choose summary. At the bottom of the page are Artifacts, where binaries can be downloaded.

Sorry. I'm not sure I quite understand what you mean?

CANopenNode commented 3 weeks ago

I just found generated binaries for checks in pull request. For binaries choose details for one of the checks, then choose summary. At the bottom of the page are Artifacts, where binaries can be downloaded.

Sorry. I'm not sure I quite understand what you mean?

It is useful for quick testing, if one does not run the Visual studio. Compiled binaries for this PR are at the bottom of this page: https://github.com/CANopenNode/CANopenEditor/actions/runs/11608883802

CANopenNode commented 3 weeks ago

Does anyone know why particulate default values that are not set trigger a warning when saving the project file? EDSChk from Vector has no problem with this (EDS, XDD1.0 checked)

There are warnings for CANopenNode_Legacy. Otherwise I get no warnings, when using file with some undefined default values: https://github.com/CANopenNode/CANopenDemo/blob/master/demo/demoDevice.xdd Which warnings do you get?

trojanobelix commented 3 weeks ago

image But when generating od.x, not when saving the project.

trojanobelix commented 3 weeks ago
                bool setDefaultValueToAll = false;
                string oldDefaultValue ="";
                if (od.parent != null && od.Subindex > 0 )
                {
                    DialogResult confirm = MessageBox.Show("Do you want to set all identical default values in subobjects to this default value?", "Set to all?", MessageBoxButtons.YesNo);
                    if (confirm == DialogResult.Yes)
                    {
                        setDefaultValueToAll = true;
                        oldDefaultValue = od.defaultvalue;

                    }
                }
                if (setDefaultValueToAll)
                {
                    for (ushort i = 1; i < od.parent.Nosubindexes; i++)
                    {
                        if (od.parent.subobjects[i].defaultvalue == oldDefaultValue)
                        {
                            od.parent.subobjects[i].defaultvalue = textBox_defaultValue.Text;
                        }
                    }
                }

Once again you discuss, because I just had to change a default value from -6300 to 49999 with 16 subs :-) Came, also already more often and have done this directly in the XDD

The code above allows you to replace all the same default values (including empty once). Seems simple and covers both use cases.

CANopenNode commented 3 weeks ago

image But when generating od.x, not when saving the project.

This is warning from canopennode_v4 exporter, specific for array. Stack requires, that all members must have some similar properties. Including default value set or not set on all of them.

CANopenNode commented 3 weeks ago
                bool setDefaultValueToAll = false;
                string oldDefaultValue ="";
                if (od.parent != null && od.Subindex > 0 )
                {
                    DialogResult confirm = MessageBox.Show("Do you want to set all identical default values in subobjects to this default value?", "Set to all?", MessageBoxButtons.YesNo);
                    if (confirm == DialogResult.Yes)
                    {
                        setDefaultValueToAll = true;
                        oldDefaultValue = od.defaultvalue;

                    }
                }
                if (setDefaultValueToAll)
                {
                    for (ushort i = 1; i < od.parent.Nosubindexes; i++)
                    {
                        if (od.parent.subobjects[i].defaultvalue == oldDefaultValue)
                        {
                            od.parent.subobjects[i].defaultvalue = textBox_defaultValue.Text;
                        }
                    }
                }

Once again you discuss, because I just had to change a default value from -6300 to 49999 with 16 subs :-) Came, also already more often and have done this directly in the XDD

The code above allows you to replace all the same default values (including empty once). Seems simple and covers both use cases.

I agree with this change. But in this case we should add additional for loop before showing MessageBox, as suggested by @nimrof. Otherwise MessageBox-es will be too frequent.

trojanobelix commented 3 weeks ago

Okay, the last commit should do the trick.

trojanobelix commented 2 weeks ago

I have added the option of selecting several subindexes in the list view and then setting the default value for the selected ones.