Firmwareupdater – Add support for other processes on multi-core boards
#850
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
davidetome
approved these changes
Dec 30, 2022
marcoaccame
reviewed
Jan 13, 2023
| @@ -1001,6 +1008,8 @@ void MainWindow::onAppendInfo(boardInfo2_t info,eOipv4addr_t address) | |||
| infoTreeWidget->addTopLevelItem(bootStrapNode); | |||
| infoTreeWidget->addTopLevelItem(propertiesNode); | |||
|
|
|||
| constexpr uint8_t maxNumberOfProcessesOnSingleCore {3}; | |||
There was a problem hiding this comment.
I can see that later on we have a switch(info.boardtype) where it is missing the case of amc.
Some comments:
- the string name in here is wrongly obtained by a switch case and we don't have
amc. That is a wrong implementation. Because there is another mode of obtaining the string and is by using the relevant functioneoboards_type2string()inside icub-firmware-shared. We should stick to this latter otherwise we duplicated code and we shall surely forget to change the code in here when new boards come along. - At the same mode, if we want to know if the board has two cores, it is wise to add a function inside icub-firmware-shared. Something as:
eoboards_type2numberofcores().
There was a problem hiding this comment.
fixed (partially?) in f0b7ded
marcoaccame
reviewed
Jan 13, 2023
marcoaccame
reviewed
Jan 13, 2023
| @@ -815,8 +866,8 @@ std::string EthMaintainer::processMoreInfoReplies(EthBoardList &boardlist) | |||
|
|
|||
| while(mSocket.ReceiveFrom(rxipv4addr, rxipv4port, mRxBuffer, sizeof(mRxBuffer), 500) > 0) | |||
| { | |||
| eOuprot_cmd_MOREINFO_REPLY_t *moreinfo = (eOuprot_cmd_MOREINFO_REPLY_t*) mRxBuffer; | |||
There was a problem hiding this comment.
Just a memo to myself for later: so in here we don't need / manage a eOuprot_cmd_MOREINFO2_REPLY_t, or do we?.
marcoaccame
reviewed
Jan 13, 2023
| @@ -1066,6 +1115,112 @@ std::string EthMaintainer::prepareMoreInfoText(eOuprot_cmd_DISCOVER_REPLY_t * di | |||
| return info; | |||
| } | |||
|
|
|||
| std::string EthMaintainer::prepareMoreInfoText(eOuprot_cmd_DISCOVER_REPLY2_t * disc, const char *ipv4rxaddr_string) | |||
There was a problem hiding this comment.
Just a memo to myself: have a look at that.
marcoaccame
reviewed
Jan 13, 2023
| @@ -22,6 +22,7 @@ const int EthUpdater::partition_UPDATER = uprot_partitionUPDATER; | |||
| #define PRINT_DEBUG_INFO_ON_TERMINAL | |||
|
|
|||
|
|
|||
| // TODO: check here if it is called and when (it seems that it's never called) | |||
There was a problem hiding this comment.
Just a check to myself: Is this code called by the FirmwareUpdater?
- Resolve some todos simplifying the code - Remove unused dead code - Fix indentation - Fix comparisons between different naive types
- These changes allow the `FirmwareUpdater` to discover and update the processes running on multicore boards such as the `AMC` boards family
- replace `else if` instead of `if` in `EthMaintainer` for `uprot_OPC_DISCOVER2` - get the string type of a board using the convert function instead of checking manually the `info.boardtype` structure.
|
Hi @sgiraz, I will merge these two linked PRs:
and you should put this PR in ready to review state. @pattacini: the PR is ok by me |
|
Awaiting the CI before merging. |
|
CI failed, @sgiraz. Could you please have a look at the report? Version of dependencies used: |
…ed` version 1.30.0
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What's new:
main changes
eOuprot_cmd_DISCOVER_REPLY2_tto parse the reply coming from the AMC multi-core family boards.uprot_OPC_DISCOVER2help theprocessDiscoveryReplies2to discriminate the parsing strategy, maintaining the back-compatibility with the existing ones.prepareMoreInfoTextnow parses the info contained in theextraprocesseswhen they exist.minor changes
cmake_minimum_requiredto VERSION 3.12Erase Applicationbutton is visible when running theFirmwareupdatewith the administrator privileges (-a)EthUpdater.cppNotes:
Erase Applicationis currently a work in progress, so at the moment it does nothing (it is disabled by default)cc @pattacini @marcoaccame @mfussi66 @davidetome