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: git commit -m " Improvements and extensions for the system clock" #121

Closed
wants to merge 13 commits into from

Conversation

Jos-Ven
Copy link
Contributor

@Jos-Ven Jos-Ven commented Sep 17, 2023

Also added a list with the most important differences compared to the main branch.

@quozl
Copy link
Collaborator

quozl commented Sep 18, 2023

I don't understand. PlatformIO is just one build choice, there's no reason to have a branch that specifically disables it, because you can avoid it easily, so the changes to README.md here in this branch make no sense to me.

@Jos-Ven
Copy link
Contributor Author

Jos-Ven commented Sep 18, 2023

Hi, As far as I understand the main branch has extra code in platformio.ini file
and in the directory tree ~/cforth/.pio in order to run 'pio run'
They are not present in the WIP branch.

pio run # In the WIP branch tells:
NotPlatformIOProjectError: Not a PlatformIO project. platformio.ini file has not been found in current working directory (/home/pi/cf/cforth). To initialize new project please use platformio project init command

However I see a indeed ~/cforth/src/platform directory in the WIP branch.
Would it be acceptable to you if I remove the line with:
"It does not use PlatformIO"?

@MitchBradley
Copy link
Owner

quozl maintains the platformio setup so it is up to him how to handle that.

@MitchBradley
Copy link
Owner

The src/platform directory is unrelated to PlatformIO. It existed long before PlatformIO was invented.

@MitchBradley
Copy link
Owner

The only file that PlatformIO looks for initially is platformio.ini . I think that PlatformIO will automatically create the .pio directory if it does not exist. .pio is where PlatformIO stores compilation artifacts like .o files, auto-fetched libraries from dependencies, and binaries. If you run pio run -t clean, it will delete everything under .pio and start from scratch.

@quozl
Copy link
Collaborator

quozl commented Sep 18, 2023

.pio is in .gitignore in the master branch. It is a build artifact.

@Jos-Ven, you should cherry-pick and merge the PlatformIO addition commits from master into WIP to make your branch closer to master. I did that for you earlier when I rebased your branch to create #113.

@Jos-Ven
Copy link
Contributor Author

Jos-Ven commented Sep 19, 2023

@quozl In 113> I propose the WIP can be added to master
Promoting WIP to master is also what I prefer.

merge the PlatformIO addition commits from master into WIP

I am afraid that it will cause many problems that I can not solve.

@MitchBradley
Copy link
Owner

It occurs to me that we are going about this the wrong way. Instead of having a branch with extensive mods to the esp32 build target, it would be better to make a different build target like esp32-jos (or another name that is more description of the functionality). That would isolate build methodology changes.

@quozl
Copy link
Collaborator

quozl commented Sep 19, 2023

@Jos-Ven wrote:

I am afraid that it will cause many problems that I can not solve.

Given that the reverse was not true; that your commits applied to my branch did not break my build using PlatformIO, it is likely that applying my commits to your branch also won't break anything. And if it does break anything, then we want to know, so we can resolve it together. I suggest you apply the commits one by one to a new branch and find out what goes wrong.

@MitchBradley wrote:

make a different build target like esp32-jos

Yes, that would avoid extensive change to the esp32 build target, yet still allow @Jos-Ven's work to be widely appreciated in the repository.

@Jos-Ven
Copy link
Contributor Author

Jos-Ven commented Sep 19, 2023

I just tried the different build target option.
After some changes the application ntc-web for the ESP32 works!
I will push the updates to the main branch.
I choose esp32-ex as target name to indicate the extra options for the ESP32.

@MitchBradley
Copy link
Owner

  1. "ex" could also be interpreted as "previous", as in "ex-wife". Ideally I would prefer something more specific than "extra". What do you call the next version? "extra-extra". After a couple of rounds of "new and improved", it ceases to have meaning. "esp32-jos" would be better than "-ex" because at least it identifies that you are the creator.

@Jos-Ven
Copy link
Contributor Author

Jos-Ven commented Sep 19, 2023

No problem, I will change it tomorrow in: esp32-extra

@quozl
Copy link
Collaborator

quozl commented Sep 19, 2023

Also if there are things you haven't changed, like build/*/sdk_build, find a way to use the files in ../esp32 instead. That will avoid duplication.

@Jos-Ven
Copy link
Contributor Author

Jos-Ven commented Sep 20, 2023

@quozl That is too complicated for me.
There are also options and setting that may have an impact in the future.
EG: When the new target is updated to a newer version of the SDK.

@Jos-Ven
Copy link
Contributor Author

Jos-Ven commented Oct 2, 2023

See #122

@Jos-Ven Jos-Ven closed this Oct 2, 2023
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