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

No banner #3026

Merged
merged 2 commits into from
Feb 9, 2020
Merged

No banner #3026

merged 2 commits into from
Feb 9, 2020

Conversation

532910
Copy link
Contributor

@532910 532910 commented Jan 31, 2020

Fixes #3021.

Make sure all boxes are checked (add x inside the brackets) when you submit your contribution, remove this sentence before doing so.

  • This PR is for the dev branch rather than for master.
  • This PR is compliant with the other contributing guidelines as well (if not, please describe why).
  • I have thoroughly tested my contribution.
  • The code changes are reflected in the documentation at docs/*.

<Description of and rationale behind this PR>

@532910 532910 mentioned this pull request Jan 31, 2020
@TerryE
Copy link
Collaborator

TerryE commented Jan 31, 2020

@532910 Sergio, you need to read the contribution guidelines. The main issues here are:

  1. I don't think that we have a consensus on the requirements so a PR is premature IMO.
  2. You need to read the contribution guidelines, and in particular that you should base your changes on a pull of the latest dev version and not you potentially out of date version.
  3. IMO the comment in user_config.his far too verbose.

But let's get one sorted first.

@marcelstoer marcelstoer changed the base branch from master to dev January 31, 2020 21:15
@marcelstoer
Copy link
Member

I fixed 2. by changing the base from master to dev here on GitHub.

add x inside the brackets

This was ignored so I fixed that as well BUT

PR is premature IMO

is still valid of course.

@532910
Copy link
Contributor Author

532910 commented Feb 9, 2020

  1. Agree!
  2. I've read https://github.com/nodemcu/nodemcu-firmware/blob/master/CONTRIBUTING.md
    Could you explain what have I done wrong? The steps I did:
  • Forked from github web interface
% git clone git@github.com:532910/nodemcu-firmware
% cd nodemcu-firmware
% git checkout -b no-banner origin/dev
# modified files
% git push origin HEAD
  • PR via github web interface

@marcelstoer
Copy link
Member

Could you explain what have I done wrong? The steps I did:
...
PR via github web interface

When you do this you need to select the "base" branch i.e. the one you want the PR to be merged into - which is normally the same you branched from. GitHub always suggests the default branch but you need to change that to dev. https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/creating-a-pull-request#creating-the-pull-request -> step 4

You can easily verify that because the list of commits/changes in the PR is really long if you select the wrong base branch. If you picked the wrong one you can hit the "Edit" button at the top-right and correct it. That's what I fixed.

@nwf
Copy link
Member

nwf commented Feb 9, 2020

Just as a point of interest, in the non-interactive test framework I've been prototyping, I rely on the startup banner to detect when the device has booted as expected or rebooted unexpectedly. I'm not sure why its presence would be a problem. Moreover, I'm jittery about removing it entirely given that we get so many bug reports from people who really need to read it (#2903 is still raw in my mind) -- if it were gone, there'd be no chance of them reading it, rather than merely a slim chance.

@532910
Copy link
Contributor Author

532910 commented Feb 9, 2020

@nwf, see #3021. The banner is for users and development process. But it's bad for other devices.
For example I have a serial screen and I want no messages to be send to it without my permission.

@marcelstoer
Copy link
Member

I'm ok with this as long as it's enabled by default.

@532910
Copy link
Contributor Author

532910 commented Feb 9, 2020

It should be enabled by default, of course.

@nwf
Copy link
Member

nwf commented Feb 9, 2020

This PR looks fine to me (though I might suggest changing "very helpful for bug reports" to "mandatory for bug reports"), but...

You will probably be better served by changing the hardware a bit:

  • a bit-bang UART on a GPIO entirely under your control,
  • an AND gate and a GPIO gating one of the existing hard UARTs, or
  • a dedicated UART chip (e.g. via I2C and a SC16IS740) that won't be spoken to without your permission.

@marcelstoer marcelstoer added this to the Next release milestone Feb 9, 2020
@marcelstoer marcelstoer merged commit 9fb8a2f into nodemcu:dev Feb 9, 2020
@532910
Copy link
Contributor Author

532910 commented Feb 9, 2020

an AND gate and a GPIO gating one of the existing hard UARTs, or

Yes, that's exactly what I did.

@532910
Copy link
Contributor Author

532910 commented Feb 9, 2020

"very helpful for bug reports" to "mandatory for bug reports"

agree

@532910
Copy link
Contributor Author

532910 commented Feb 10, 2020

When you do this you need to select the "base" branch i.e. the one you want the PR to be merged into - which is normally the same you branched from.

Clear, Marcel thank you for comprehensive explanation!

marcelstoer pushed a commit that referenced this pull request Jun 9, 2020
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

4 participants