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

Update Segger SysView files to v3.10 #437

Merged
merged 6 commits into from
Mar 6, 2020
Merged

Conversation

henrygab
Copy link
Collaborator

@henrygab henrygab commented Feb 11, 2020

@hathach -- This is a precursor to any further work on #431. It focuses on updating the Segger SystemView / RTT files, without changing their use. Two minor bug fixes also included.

  1. Adds /boards.local.txt and /platform.local.txt to .gitignore, to prevent accidental pushes of those files.

  2. Copies SystemView 3.10 files over the existing versions, including adding two new files SEGGER_RTT_printf.c and SEGGER_RTT_ASM_ARMv7M.S.
    Note: Compilation at this commit fails with a linker error, undefined reference to SEGGER_RTT_ASM_WriteSkipNoLock.

  3. Restore existing pre-PR modifications to SEGGER files, to disable use of the ASM version of the function, thus avoiding the linker error.

  4. Applies the FreeRTOSv10_Core.patch file, modified as needed for this depot.

  5. Restore existing pre-PR modifications to SEGGER files, so SysView functions have no implementation when debug level is less than < 3.

  6. Fix compilation warning because it should always be clean building with warnings enabled.

😉 Please accept my most sincere apologies for being unable to update the SEGGER files by simply copying them. It appears that someone made modifications to the previously used SEGGER files, thus preventing me from honoring your request to avoid (absolutely and under no circumstances) making any changes to those files, while still having the system build properly. 😉

boards.local.txt and platform.local.txt should
never exist in main branch.
Note: This does not compile, which is expected.
Need to apply patch file for RTOS v10 and restore
existing configuration modifications.
When linking, get errors about undefined references
to `SEGGER_RTT_ASM_WriteSkipNoLock`
Of course, had to apply them manually, as the patch file
did not apply directly.  This is because some files were
renamed / moved relative to the original distribution,
while others were deleted because they did not apply.
size_t is unsigned; the #defined constant was treated as an integer,
which caused the following warning:

In file included from cores\nRF5\Uart.cpp:21:0:
cores\nRF5\Arduino.h: In instantiation of
```C++
  decltype ( ((b < a) ? b :  a) )
   min(const T&, const L&)
   [with
    T = unsigned int;
    L = int;
    decltype (((b < a) ? b :  a)) = unsigned int
   ]:
```

cores\nRF5\Uart.cpp:228:54: required from here
cores\nRF5\Arduino.h:92:15: warning:
comparison between signed and unsigned integer expressions [-Wsign-compare]
     return (b < a) ? b : a;
@henrygab henrygab changed the title [WIP] Update Segger SysView files to v3.10 Update Segger SysView files to v3.10 Feb 12, 2020
@henrygab
Copy link
Collaborator Author

@hathach -- I would very much like your review / approval. I think this is as close as possible to your request to copy the Segger files, and in fact includes a commit that actually did exactly that -- only copying the new files over.

@hathach
Copy link
Member

hathach commented Feb 22, 2020

@henrygab thank you very much for your effort, I am currently on a tight schedule for other works. I will review this as soon as I could. Please be patient a little more :)

@henrygab
Copy link
Collaborator Author

henrygab commented Mar 6, 2020

Keeping this thread alive.

@hathach
Copy link
Member

hathach commented Mar 6, 2020

That is coincident, I am reveiwing this just now 😃😃

Copy link
Member

@hathach hathach left a comment

Choose a reason for hiding this comment

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

Superb!! Thank you very much for the PR. And sorry for the late response. We are releasing CLUE & Sense boards which are packed with sensor and TFT and lots of more stuffs, those get high priority to support.

Now, I somewhat remembered, I was a bit nervous with modifying FreeRTOS kernel at first. I guess we will miss-out several kernel events, but considered that is not worth the risk. Lots of thing could go wrong since that is initial work for the BSP.

I still hate modifying the kernel, but I am more confident now, since there is more set of eyes (yours) watching on this. If anything goes wrong, I will just nuke the modification with latest freeRTOS 10.3 😈

@hathach
Copy link
Member

hathach commented Mar 6, 2020

Please accept my most sincere apologies for being unable to update the SEGGER files by simply copying them. It appears that someone made modifications to the previously used SEGGER files, thus preventing me from honoring your request to avoid (absolutely and under no circumstances) making any changes to those files, while still having the system build properly.

That is me, I could do better by excluding the files from compiling but the Arduino IDE doesn't allow to such things 😜

@hathach hathach merged commit 569b9b2 into adafruit:master Mar 6, 2020
@henrygab
Copy link
Collaborator Author

henrygab commented Mar 6, 2020

We are releasing CLUE & Sense boards which are packed with sensor and TFT and lots of more stuffs

Yes, indeed! Not to mention the nrf52840-based metro express that's coming soon and already having a CircuitPython 5.0 release. Truly a busy time!

[prior commit had modified files comment]

That is me, I could do better by excluding the files from compiling but the Arduino IDE doesn't allow to such things 😜

Yes, the Arduino environment does present some challenges. Thank you for accepting the PR.

@henrygab henrygab deleted the Segger_RTT_3 branch March 11, 2020 22:30
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.

2 participants