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

Wip #115

Open
wants to merge 42 commits into
base: master
Choose a base branch
from
Open

Wip #115

wants to merge 42 commits into from

Conversation

MitchBradley
Copy link
Owner

No description provided.

root and others added 30 commits September 25, 2021 13:57
ESP32 ADC improvements
…equency of the CPU is changed with rtc-clk-cpu-freq-set.
Jos: Additions for calls for an extra uart and lwip-recv-r
@MitchBradley MitchBradley requested a review from quozl August 9, 2023 12:56
@quozl
Copy link
Collaborator

quozl commented Aug 10, 2023

The branch predates my work on PlatformIO so unable to test it directly, but did cherry-pick all later commits onto my rebased try-wip branch on #113 and did build the pico firmware without error. This is not a test of the new code, but rather a test that my builds don't fail once the new code is merged.

Copy link
Collaborator

@quozl quozl left a comment

Choose a reason for hiding this comment

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

Not sure why .f files are used instead of .fth, but probably not important.

@Jos-Ven
Copy link
Contributor

Jos-Ven commented Aug 10, 2023

*.f files are for me an indication that they also run under Gforth and Win32Forth.
I hope that it is OK to use these extensions.

@quozl
Copy link
Collaborator

quozl commented Aug 10, 2023 via email

@Jos-Ven
Copy link
Contributor

Jos-Ven commented Aug 10, 2023

The branch predates my work on PlatformIO....
I can also copy the changed files to my main branch on my PC test them and adapt what is needed.
Then when they are alright, push them to the main branch of Mitch.
Would that be alright?

@quozl
Copy link
Collaborator

quozl commented Aug 10, 2023

There is much you can do, but it is up to you whether to do it. I can give you ideas, but you need to be comfortable with using git, either with the repository you are already using or with a backup.

You could;

  • fetch, check out and test my try-wip branch, see if it does everything you need, and if so review my pull request and approve it, or;
  • visualise the history (e.g. gitk --all or git log) and use git cherry-pick to bring in to your branch the changes to master that happened in the time since you departed from it, or;
  • rebase in the same manner that I did when I created the try-wip branch.

On the other hand, Mitch might be fine with just merging as is. Up to him. I find histories with many merge commits problematic because git bisect is harder, but there are people who don't mind that.

@MitchBradley
Copy link
Owner Author

I have worked on projects that insisted on linear history, avoiding merge commits like the plague, and ones that just hit merge and lets the chips fall where they may. The former is probably better when lots of people are involved, the latter likely easier for small teams. I don't have a strong preference either way.

@quozl
Copy link
Collaborator

quozl commented Aug 10, 2023

@MitchBradley, thanks.

@Jos-Ven, what is your view on linear history vs merge commits?

@Jos-Ven
Copy link
Contributor

Jos-Ven commented Aug 10, 2023

I am under the impression that merge commits might lead into problems.

Keeping history is not so important for me.
I still can see what has been changed or added.
Some c-parts could be implemented better.
When it is up to me, then I would like to push the changes with possible improvements to the main branch.
Ok?

@MitchBradley
Copy link
Owner Author

PR to master is okay with me.

@Jos-Ven
Copy link
Contributor

Jos-Ven commented Aug 11, 2023

PR to master is okay with me.
I also agree and will test the result.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants