Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add board WiFi Slot #3916

Merged
merged 7 commits into from
Jun 4, 2018
Merged

add board WiFi Slot #3916

merged 7 commits into from
Jun 4, 2018

Conversation

acosinwork
Copy link
Contributor

Added new board WiFi Slot with esp12-f.
Description: http://wiki.amperka.ru/%D0%BF%D1%80%D0%BE%D0%B4%D1%83%D0%BA%D1%82%D1%8B:wifi-slot

@d-a-v
Copy link
Collaborator

d-a-v commented Dec 5, 2017

Check #3722. New boards in boards.txt will be integrated through a script. In order to add your board, and what is not clear enough in your files,

  • is the flashing method, is it dout/qio/dio or qout ? (or should we leave all of them with dout as default)
  • is the flash frequency 40 or 80Mhz ?
  • serial speed default is 115k, but would you prefer 921k as default ?

@acosinwork
Copy link
Contributor Author

acosinwork commented Dec 5, 2017

Hi, thank you.
Only now I found out that our module easily supports these modes.

  • we can set qio as as default. But we must leave all of them accessible.
  • 80 Mhz as default - ok. It will be great. But I would leave the opportunity to choose 40 Mhz
  • 921k as default - ok. And leave 115200 possible for a choice

Sorry, I did not really understand. Do I have to change the boards.txt now?

@d-a-v
Copy link
Collaborator

d-a-v commented Dec 5, 2017

Boards.txt will be generated by a script. Boards are entries in an array
If you leave all options as user-definable, then what is the difference with the generic board ?

@acosinwork
Copy link
Contributor Author

For this board is important that reset method is nodemcu, custom .build.board (wifi_slot in variants) and fash size is more than 1M. I really liked your suggestions for the default settings. But I can not be sure that all the family esp12 will be stable in the future

@acosinwork
Copy link
Contributor Author

Should I change the script in your branch?

@d-a-v
Copy link
Collaborator

d-a-v commented Dec 6, 2017

Your new board brings more changes than a simple new board declaration.
So declaring it in the script in it will make it half integrated and not usable.
I apologize I came too fast to you and did not look at your other changes.
So if your changes are integrated before the generator, then I will update it.
If the generator is integrated before, then you'll have to adapt your pull request.

static const uint8_t A6 = PIN_A6;
static const uint8_t A7 = PIN_A7;

static const uint8_t LED_BUILTIN = 2;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use a #define LED_BUILTIN like in the other variants and get rid of BUILTIN_LED, it is defined in common.h that you should use to avoid duplicate code.

Is that fair ?

Copy link
Contributor Author

@acosinwork acosinwork May 31, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your feedback.
I'll fix the #define LED_BUILTIN. Unfortunately, I can not just connect common.h, because the pinout of our board conflicts with this:

#define PIN_A0 (17)
static const uint8_t A0 = PIN_A0;

I could change the common.h in this part to this code:

#ifndef PIN_A0
#define PIN_A0 (17)
#endif

static const uint8_t A0 = PIN_A0;

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any problem with this proposal,
as long as the API remains compatible with the Arduino use of analogRead(A0)
which seems to be the case in your PR and your new analog pins and I guess a analog multiplexer.

@acosinwork
Copy link
Contributor Author

@d-a-v , it seems that the code has magically passed the tests. Could you look at this please?

@d-a-v d-a-v merged commit 9c5c16e into esp8266:master Jun 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants