Skip to content

Conversation

@Rautanyrkki
Copy link

Added the sensoan/sgw2 board
Added 'sensoan Sensoan Oy' to vendor-prefixes.txt

@Rautanyrkki
Copy link
Author

Rautanyrkki commented Dec 2, 2024

The pull request draft ended up having many fix attempts, and I am not sure whether just pushing them one after another was the correct procedure. I envisioned that in a passing pull request all the commits would be squashed into a single commit having the the comment that I gave in the original commit, and the later comments would be ignored. This is the reason why the comments of the later commits do not have the correct format (and are not always particularly informative).
This has been fixed as suggested by nordicjm.

@Rautanyrkki Rautanyrkki marked this pull request as ready for review December 2, 2024 13:17
@zephyrbot zephyrbot requested review from decsny and galak December 2, 2024 13:18
@Rautanyrkki Rautanyrkki requested a review from nordicjm December 2, 2024 13:19
Comment on lines 9 to 10
Copy link
Contributor

Choose a reason for hiding this comment

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

2 space indent

Copy link
Author

Choose a reason for hiding this comment

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

Done

Comment on lines 42 to 49
Copy link
Contributor

Choose a reason for hiding this comment

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

don't really see why just having the SPI NOR device in dts should enable SPI NOR, multithreading and flash, nor why there should be a Kconfig for this. If this is for MCUboot then submit a config file for MCUboot directly to the repo

Copy link
Author

Choose a reason for hiding this comment

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

Removed

Copy link
Contributor

Choose a reason for hiding this comment

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

text is not aligned

Copy link
Author

@Rautanyrkki Rautanyrkki Dec 4, 2024

Choose a reason for hiding this comment

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

I am not sure what you mean. To me this looks similar to e.g. the arduino_header node in boards/nordic/nrf9160dk/nrf9160dk_nrf9160_common.dtsi. I did make some changes in the indentation of lines in the gpio-map field.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you check in the review tab here https://github.com/zephyrproject-rtos/zephyr/pull/81437/files#diff-3b7bdd3eba91830df7d72e7821cef67abb42fccc74502b65af274ef3dc169f06R72 the green and blue options are indented more than the rest so they stick out, not required to fix anyhow

Copy link
Author

Choose a reason for hiding this comment

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

Indentation fixed.

Copy link
Contributor

Choose a reason for hiding this comment

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

text is still not aligned but not a requirement to fix anyhow, just looks odd

@Rautanyrkki Rautanyrkki force-pushed the add_sgw2_board branch 2 times, most recently from a704847 to 827af1f Compare December 4, 2024 14:13
@Rautanyrkki Rautanyrkki requested a review from nordicjm December 4, 2024 14:16
@zephyrbot zephyrbot requested a review from rruuaanng December 9, 2024 09:20
@Rautanyrkki Rautanyrkki force-pushed the add_sgw2_board branch 3 times, most recently from 6a87d9a to 89092f1 Compare December 11, 2024 07:48
@decsny decsny removed their request for review December 11, 2024 13:21
@Rautanyrkki
Copy link
Author

It seems that "Run tests with twister / twister-build (9)" has started to fail (due to build failure of sample.net.sockets.http.server) even though it has passed earlier. Based on this commit 8f07784 it seems that the test itself has been broken at some point. Is there a way to cause the tests rerun without pushing changes to the pull request? And are the run twister tests always taken from the current main branch?

Added 'sensoan Sensoan Oy' to vendor-prefixes.txt

Signed-off-by: Johan Kopra <[email protected]>
nordicjm
nordicjm previously approved these changes Dec 17, 2024
Copy link
Contributor

@nordicjm nordicjm left a comment

Choose a reason for hiding this comment

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

Some nits. changes are OK

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

Copy link
Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

Copy link
Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

Choose a reason for hiding this comment

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

text is still not aligned but not a requirement to fix anyhow, just looks odd

@nordicjm nordicjm requested review from anangl and kartben December 17, 2024 12:19
@Rautanyrkki
Copy link
Author

It seems that "Run tests with twister / twister-build (9)" has started to fail (due to build failure of sample.net.sockets.http.server) even though it has passed earlier. Based on this commit 8f07784 it seems that the test itself has been broken at some point. Is there a way to cause the tests rerun without pushing changes to the pull request? And are the run twister tests always taken from the current main branch?

Test remains failing after rebase and push.

Added the sensoan/sgw2 board

Signed-off-by: Johan Kopra <[email protected]>
Copy link
Contributor

@nordicjm nordicjm left a comment

Choose a reason for hiding this comment

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

Missed the int on this selection, non-default/new Kconfigs need to go into a Kconfig file

Comment on lines +98 to +100
config HEAP_MEM_POOL_ADD_SIZE_BOARD
int
default 4096 if BT_HCI_IPC
Copy link
Contributor

Choose a reason for hiding this comment

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

needs to move to Kconfig

@github-actions
Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Mar 18, 2025
@kartben
Copy link
Contributor

kartben commented Mar 18, 2025

@Rautanyrkki will you be able to come back to this pull request?

@github-actions github-actions bot removed the Stale label Mar 19, 2025
@github-actions
Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label May 18, 2025
@github-actions github-actions bot closed this Jun 1, 2025
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.

4 participants