search

RedBearLab / nRF51822-Arduino

Moved to https://github.com/redbear/nRF5x
251 stars 109 forks source link

Fundamental Arduino classes should not be casually renamed, e.g. Stream --> WStream #46

Open george-hawkins opened 8 years ago

george-hawkins commented 8 years ago

Being able to compile the same code on multiple MCUs with little or no change is one of the big pluses of the Arduino ecosystem.

By renaming core classes in the base system of Arduino libraries you are breaking this and so reducing the value of all your Arduino compatibility work.

An example of this is the Arduino Stream class which has been renamed WStream here.

I understand that you did this because you've started to include mbed classes, and this renaming avoids a clash with an identically named class from mbed.

However this is the nRF51822-Arduino repo - if any code had to be changed surely it should have been the mbed code and not the core Arduino code?

If people have to investigate unexpected changes to basic classes like this (and wonder why there code seems to be picking up unrelated mbed logic instead of what they expected) then this gives quite a negative impression :disappointed:

I hope RBL's products are a great success, including the recent Kickstarter. The development tools are an important part of this. The Arduino system has a lot of issues compared to more sophisticated tools but they are popular in the hobbyist space. Please think carefully before breaking compatibility, in terms of class names etc., in this space.

And good luck with all your work :smile:

sandeepmistry commented 8 years ago

However this is the nRF51822-Arduino repo - if any code had to be changed surely it should have been the mbed code and not the core Arduino code?

@george-hawkins I agree. I took this approach when I submitted #41, and renamed Serial1 back to Serial.

soundanalogous commented 8 years ago

I ran into this today as well. I have no idea why the mbed files are included with the RBL_nRF51822 library (I assume it's to share some common files but the tradeoff does not seem to be worth it). My recommendation for the RBL developers is to separate the mbed package into a separate repo if possible.

Cheong2K commented 8 years ago

I agree, too. We will start to re-structure this along with our new nRF52 (ARM Cortex-M4) board from March.

soundanalogous commented 7 years ago

Any update on this? I haven't seen any changes. It would be helpful to resolve the conflicts due to the renaming of Stream.h to WStream.h. This repo is named "nRF51822-Arduino" not "nRF51822-mbed" so it really doesn't make sense to rename core Arduino library files in a way that breaks compatibility with many existing libraries (any Arduino library that includes Stream.h). The only reason I can see for the renaming was so that the mbed version of Stream.cpp didn't need to be renamed. Makes no sense however given the intent of this repo as being for Arduino.