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.

Sign up for GitHub

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

Jump to bottom

Two small patches for build system and fatfs (IDFGH-10849) #12052

Merged
merged 2 commits into from
Aug 18, 2023
Merged

Two small patches for build system and fatfs (IDFGH-10849) #12052

merged 2 commits into from
Aug 18, 2023

Conversation

jendo42
Copy link
Contributor

@jendo42 jendo42 commented Aug 10, 2023

  • idf.py gdb action incorrectly generated EOL in gdbinit
  • fatfs routines incorrectly report state for read-only filesystem

@CLAassistant
Copy link

CLAassistant commented Aug 10, 2023 •
edited
Loading

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 20 committers have signed the CLA.

✅ jendo42
✅ hfudev
❌ 10086loutianhao
❌ esp-cjh
❌ ESP-Marius
❌ dobairoland
❌ IshaESP
❌ esp-jiangguangming
❌ Dazza0
❌ nileshkale123
❌ suda-morris
❌ radek.tandler
❌ rahult-github
❌ sudeep-mohanty
❌ 0xjakob
❌ Lindazhxy
❌ radimkarnis
❌ KonstantinKondrashov
❌ mahavirj
❌ Kainarx

radek.tandler seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@espressif-bot espressif-bot added the Status: Opened Issue is new label Aug 10, 2023
@github-actions github-actions bot changed the title Two small patches for build system and fatfs Two small patches for build system and fatfs (IDFGH-10849) Aug 10, 2023
@jendo42 jendo42 force-pushed the devel branch 2 times, most recently from 87a7dd2 to 5220b0d Compare August 10, 2023 10:44
igrr
igrr reviewed Aug 10, 2023
View reviewed changes
Copy link
Member

@igrr igrr left a comment

Choose a reason for hiding this comment

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

Thank you for the PR! Fatfs change looks good to me, just one question about the gdbinit part.

tools/idf_py_actions/debug_ext.py
@@ -270,7 +270,7 @@ def generate_gdbinit_rom_add_symbols(target: str) -> str:
r.append('set confirm on')
r.append('end')
r.append('')
return os.linesep.join(r)
return '\n'.join(r)
Copy link
Member

Choose a reason for hiding this comment

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

Did os.linesep cause any issue when you used it? According to the docs it should be the correct line ending depending on the host OS.

@igrr
Copy link
Member

igrr commented Aug 10, 2023

@jendo42 One more request, could you please target master branch and not release/v5.1? Features and bug can be accepted on master branch only. We can backport the change to a release branch after it is merged into master.

@jendo42
idf.py: gdb action incorrectly generated EOL gdbinit scripts
e9bb336 
GDB on Windows incorrectly reads EOL in the script files causing 'gdb'
action to fail.

(gdb) source .../build/gdbinit/py_extensions
(gdb) source .../build\gdbinit\symbols
add symbol table from file "...\build\bootloader\bootloader.elf"
.../build\gdbinit\symbols:6: Error in sourced command file:
Undefined command: "".  Try "help".

Forcing line separator to '\n' resolved the issue

Signed-off-by: Michal Jenikovsky <jendo@jmsystems.sk>
@jendo42
fatfs: raw diskio: Fixed handling read-only filesystem
bcda40f 
ff_ routines incorrectly reported disk state and caused whole fatfs
to lock-up when trying to write to read-only device.

Signed-off-by: Michal Jenikovsky <jendo@jmsystems.sk>
@jendo42 jendo42 changed the base branch from release/v5.1 to master August 11, 2023 10:42
@igrr
Copy link
Member

igrr commented Aug 11, 2023

sha=bcda40fcf22746f91f301bfcbba768d4a1747922

@igrr igrr added the PR-Sync-Merge Pull request sync as merge commit label Aug 11, 2023
@espressif-bot espressif-bot added Status: In Progress Work is in progress Status: Reviewing Issue is being reviewed and removed Status: Opened Issue is new Status: In Progress Work is in progress labels Aug 14, 2023
@espressif-bot espressif-bot added Status: Done Issue is done internally Resolution: NA Issue resolution is unavailable and removed Status: Reviewing Issue is being reviewed labels Aug 17, 2023
@espressif-bot espressif-bot merged commit af79a47 into espressif:master Aug 18, 2023
5 checks passed
@MiguelBarro MiguelBarro mentioned this pull request Dec 4, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@igrr igrr igrr approved these changes

Assignees

@igrr igrr

Labels
PR-Sync-Merge Pull request sync as merge commit Resolution: NA Issue resolution is unavailable Status: Done Issue is done internally
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jendo42 @CLAassistant @igrr @espressif-bot