boards: cleanup structure of wsn430 boards#8061
Conversation
53766ab to
e1bd0e3
Compare
e1bd0e3 to
4325deb
Compare
|
reabsed |
4325deb to
9be0ab2
Compare
PeterKietzmann
left a comment
There was a problem hiding this comment.
I've "tested" examples/default with a wsn430-v1_3b on the iotlab tesbed site in strasbourg and wsn430-v1_4 on euratech. "tested" simply means running the firmware on the node and "help" returned the expected output. Though I approve this PR, I'd like someone who has more experience with that platform to review the changes.
|
Does it make sense to merge this? The WSN430 are soon to be deactivated I read on Tuesday on the IoT-Lab-Mailinglist (whose archive isn't public so I sadly can't link). |
|
Not on all sites. I think it is too early to remove that port completely. |
|
Alright then. Hit the green ;-) |
|
Is this an ACK after reviewing the code? |
You already ACK'd. Is a second ACK necessary? |
|
I asked for one (above), so yes. I'm not so familiar with these platforms and their code base. |
|
Ah sorry ^^" no I think I'm even less familiar than you. |
9be0ab2 to
0f75510
Compare
|
rebased. @PeterKietzmann: as you have been running this code on the nodes, I think this makes you currently the only expert we have on these boards... So I don't think we will ever get someone else to give a better review then you did. So my proposal: @miri64 looks hard at the changes once more, and we merge this PR with both of your (more or less expert) verdicts. |
miri64
left a comment
There was a problem hiding this comment.
One coding convention issue, one proposal, otherwise the code is just moved around a little so I think if addressed (at least the coding convention part) we can merged this.
boards/common/wsn430/board_init.c
Outdated
| } | ||
|
|
||
| /* oscillator fault flag still set? */ | ||
| while ((IFG1 & OFIFG) != 0); |
There was a problem hiding this comment.
Why are you writing the while loop here without a body, but the for-loop above you are writing with (empty) body. In general loops without bodies can lead to cross-platform problems and our coding conventions state they should be with a body
There was a problem hiding this comment.
japp, this is a mistake here, will fix.
boards/common/wsn430/board_init.c
Outdated
| break; | ||
| default: | ||
| /* MCLK and SMCLK = XT2 (safe) */ | ||
| BCSCTL2 = (SELM_2 | SELS); |
There was a problem hiding this comment.
Minor optimization proposal: why not make the MCLK_... defines names for theses combinations of flags?
#define MCLK_2MHZ_SCLK_1MHZ (SELM_2 | DIVM_2 | SELS | DIVS_3)
#define MCLK_4MHZ_SCLK_1MHZ (SELM_2 | DIVM_1 | SELS | DIVS_3)
#define MCLK_8MHZ_SCLK_1MHZ (SELM_2 | SELS | DIVS_3)
#define MCLK_8MHZ_SCLK_8MHZ (SELM_2 | SELS)then you can just BSCTL2 = speed. Or does that not make any sense?
There was a problem hiding this comment.
yap, obviously :-) Should have seen this myself, thanks!
|
addressed comments |
boards/common/wsn430/board_init.c
Outdated
|
|
||
| /*---------------------------------------------------------------------------*/ | ||
| void msp430_init_dco(void) | ||
| void msp430_init_cpuclk(uint32_t speed) |
There was a problem hiding this comment.
Isn't used from extern anymore => can be static
boards/common/wsn430/board_init.c
Outdated
|
|
||
| /*---------------------------------------------------------------------------*/ | ||
| void msp430_init_dco(void) | ||
| void msp430_init_cpuclk(uint32_t speed) |
There was a problem hiding this comment.
Also, BCSCTL2 seems to be char according to most sources I was able to find (and the header files of the toolchain). So why not use uint8_t for speed?
|
fixed. |
3e3efda to
19b7560
Compare
|
Tested on both wsn430_1.3b and wsn430_1.4 with |
19b7560 to
0d798b5
Compare
|
squashed. |
0d798b5 to
70aa86e
Compare
|
ups, broke the do-while loop when squashing, fixed now. |
|
"Re"-tested on iotlab-m3 for both platforms. This time with the right commit ^^" |
|
thanks! |
rebased on #8058,alternative to #7975So when not taking the path of board revisions and just cleaning up the code tree a little bit, it seems to me we can achieve the same thing. For this PR just compare this commit from #7975 b64b8b0 with the same done in this PR e02f33f.
The changes look pretty similar, and I think we might as well get away with keeping a dedicated folder per board (variant), and just improving on the code sharing part... So I tend to close #7975 again and simply go with structural cleanups based on #8058.