-
-
Notifications
You must be signed in to change notification settings - Fork 6
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
fix(installer): checking cores before upload #79
fix(installer): checking cores before upload #79
Conversation
Added a function to check if all supported boards have their corresponding 'core' (like in arduino-cli core install) installed
…78-installer-install-board-cores-before-uploading
@@ -48,7 +48,37 @@ const getBoards = ({ | |||
}); | |||
}; | |||
|
|||
const upload = ({ dstPath, fqbn, port, onData, onClose, onError }) => { | |||
const installRequiredCores = async ({ fqbnList, onData }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK this should be a platform list not an fqbn list
const installRequiredCores = async ({ fqbnList, onData }) => { | |
const installRequiredCores = async ({ platforms, onData }) => { |
const installRequiredCores = async ({ fqbnList, onData }) => { | ||
const promises = []; | ||
for (let i = 0; i < fqbnList.length; i += 1) { | ||
const installRequiredCoresPromise = new Promise((resolve, reject) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if I understand the code you are running multiple installations concurrently but you are returning the output with the same onData
callback, so the output will be mixed. If that is the case I would prefer to have all the installation sequentially instead
export default [ | ||
{ | ||
fqbn: 'arduino:mbed_nano:nano33ble', | ||
platform: 'arduino:mbed_nano', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that the core is always the lower part of the fqbn, so we could just do a substring instead of keeping them explicitly.
const onData = (line) => | ||
dispatch({ type: 'APPEND_STDOUT', stdout: line }); | ||
try { | ||
await installRequiredCores({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could just install the cores that is needed fo the current board instead of everyone. This check could be executed right before the upload and based on the fqbn installing the code. I would also integrate the check in the upload itself.
Added a function to check if all supported boards have their corresponding 'core' (like in
arduino-cli core install
) installed