Add MY_RS485_DE_INVERSE define to invert the device enable pin polarity

[ltigges]

RS485 ICs like the SN65HVD230 (CAN Transceiver) have an low-active device enable.

This has been tested with an ATMEGA 328p and an SN65HVD230

[mkaiser]

CI tells me, that there is something wrong with the coding style

I am afraid your coding style is not entirely in line with the MySensors prefered style.
A mail with a patch has been sent to you that, if applied to your PR, will make it follow the MySensors coding standards.
You can apply the patch using:
git apply restyling.patch

I did not receive any mail. What is wrong with the doxygen comments?

[mfalkvidd]

Thanks! There is nothing wrong with the doxygen comments. The messags are a bit unclear. I've been thinking about a way to make them clearer. Perhaps adding the text "Great!" at the end of each line.

I'll see if there is some other way to get hold of the restyling patch.

[mfalkvidd]

The only similar keyword we have seems to be MY_WITH_LEDS_BLINKING_INVERSE.

Maybe we should try to be consistent with the naming, and call this keyword MY_RS485_DE_INVERSE ?

[mfalkvidd]

The patch is available at https://ci.mysensors.org/job/MySensors/job/MySensors/job/PR-1358/8/execution/node/3/ws/MySensors/restyling.patch

Looks like there was a stray space after 'else' and inconsistent indentation in two places.

[mkaiser]

thank you - now I understand. The patches including the renaming to MY_RS485_DE_INVERSE will come on monday.

By the way, I followed the instructions on how to get the boostrap-dev working.
I used WSL Ubuntu 18.04 LTS. For this special case I needed to manually compile astyle and cppcheck.

The curl links to the tar.gz packages point to outdated versions, which are too old for MySensors.
Can you adapt the instructions?

curl -L 'https://sourceforge.net/projects/astyle/files/astyle/astyle%203.1/astyle_3.1_linux.tar.gz/download' | tar xvz

curl -L 'https://sourceforge.net/projects/cppcheck/files/cppcheck/1.89/cppcheck-1.89.tar.gz/download' | tar xvz

new build command copied from https://github.com/danmar/cppcheck

sudo make MATCHCOMPILER=yes FILESDIR=/usr/share/cppcheck HAVE_RULES=yes CXXFLAGS="-O2 -DNDEBUG -Wall -Wno-sign-compare -Wno-unused-function" install

[mfalkvidd]

For wsl, astyle 3.1 is available through apt (for Ubuntu 18.04 lts and for 19.04) so we can update the instructions to use apt.

Cppcheck is still too old on 19.04, so there we should update the source instructions to use something newer (maybe we can refer to https://sourceforge.net/projects/cppcheck/files/latest/download so we don't need to update whenever new versions are released)

[mkaiser]

For wsl, astyle 3.1 is available through apt (for Ubuntu 18.04 lts and for 19.04) so we can update the instructions to use apt.

apt install did somehow not work for me (but 3.1 is -like you said - in the official repos) ... installing manually worked :) I guess the problem is in front of the computer...

Cppcheck is still too old on 19.04, so there we should update the source instructions to use something newer (maybe we can refer to https://sourceforge.net/projects/cppcheck/files/latest/download so we don't need to update whenever new versions are released)

I guess thats the best solution.. shall I try to commit a patch for that, including the updated build command?

back to the main topic:
Lennart and I tried to fix the last commits, but Jenkins still tells us about style issues.

I am afraid your coding style is not entirely in line with the MySensors prefered style.
A mail with a patch has been sent to you that, if applied to your PR, will make it follow the MySensors coding standards.
You can apply the patch using:
git apply restyling.patch

I double checked by running astyle on the files. It 'should' be working....

astyle --options=.mystools/astyle/config/style.cfg MyConfig.h
Unchanged /mnt/f/Projekte/MySensors/MySensors_wired_fork/MyConfig.h

Sorry for causing so much trouble for these small patches - we are hardware engineers used to work with svn and still need some time to arrive in the git-world :)

[mfalkvidd]

No worries :-) We're here to help.

The "rules" can seem cumbersome at the beginning, but we think that in the long run everyone will benefit from having consistent code style, naming, etc.

Strange that astyle doesn't report any problems. Maybe @fallberg knows why there is a difference between jenkins astyle and local astyle?

@mkaiser if you just run git apply restyling.patch and push, it should fix the style issues, even if the local astyle doesn't see the issues.

You are very welcome to update the instructions (a separate PR is probably easiest so we can focus on the RS485 DE inversion feature in this PR), but I can handle it if you like. I'll create a separate issue so we don't forget it.

[fallberg]

Mails are not sent because "Author: ltigges (null)". This usually indicate that the author has a mismatch between the local git email configuration and the corresponding github email configuration.

[fallberg]

Difference in astyle results could indicate a change in the version of astyle between the build server and the local development environment.

[mfalkvidd]

The new warning (documentation for unknown define MY_RS485_DE_INVERSE found) means that since the new define isn't set by default, Doxygen cannot see it, and therefore cannot generate documentation for it.

Just add the new define at https://github.com/mysensors/MySensors/blob/development/MyConfig.h#L2320

[ltigges]

Thank you fot the hint.
Should we merge all commits into one commit?

[mfalkvidd]

No need to merge. Github lets us squash into one commit when merging.

[mfalkvidd]

New Doxygen documentation looks good: https://ci.mysensors.org/job/MySensors/job/MySensors/job/PR-1358/13/Doxygen_20HTML/group__RS485SettingGrpPub.html#gae5db0040c0616a9a2455107cc4fba326

[mkaiser]

just noticed, that MY_RS485_DE_PIN is missing in the Doxygen output

Why did the CI not notice this? Should'nt all possible defines been listed in MyConfig.h

[mfalkvidd]

CI did not notice because the rules are only applied to lines that were modified.

Old "sins" are not considered blockers, because if they were considered blockers almost nobody would have the time/energy to fix everything before being able to push through a small change (such as adding MY_RS485_DE_INVERSE for example).

It would be nice to enforce everything, but doing that is unfortunately not realistic.

[mfalkvidd]

DE_PIN is now visible at https://ci.mysensors.org/job/MySensors/job/MySensors/job/PR-1358/15/Doxygen_20HTML/group__RS485SettingGrpPub.html#gae5db0040c0616a9a2455107cc4fba326

@mkaiser / @ltigges do you think you'll have more changes or is this ready for merge now?

[mkaiser]

sorry for the delay - did not check the results yesterday evening :)

we are ready - you are good to go!

[mfalkvidd]

Great! Thanks a lot for your contribution!