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

drivers: modem: hl7800: fix file open in fw update #28766

Merged
merged 2 commits into from
Oct 15, 2020
Merged

drivers: modem: hl7800: fix file open in fw update #28766

merged 2 commits into from
Oct 15, 2020

Conversation

rerickson1
Copy link
Member

File open function for firmware update requires a flags parameter to be passed in.

@rerickson1
Copy link
Member Author

Fixes #28745

@rerickson1
drivers: modem: hl7800: fix file open in fw update
ac11ff2 
File open function for firmware update
requires a flags parameter to be passed in.

Signed-off-by: Ryan Erickson <[email protected]>
@rerickson1 rerickson1 requested review from jukkar and rlubos September 29, 2020 14:47
galak
galak requested changes Sep 29, 2020
View reviewed changes
Copy link
Collaborator

@galak galak left a comment

Choose a reason for hiding this comment

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

Fix is fine, but we should add a build test to tests/drivers/build_all

@rerickson1
Copy link
Member Author

Fix is fine, but we should add a build test to tests/drivers/build_all

Sounds good. What are your thoughts on how to approach this?

  1. Add a board file overlay in the boards folder? (pinnacle_100_dvk can be used to test the HL7800 driver).
  2. Add a new hl7800.conf that exercises all HL7800 driver options? testcase updated to use hl7800.conf with board pinnacle_100_dvk.

@galak
Copy link
Collaborator

galak commented Sep 29, 2020

Sounds good. What are your thoughts on how to approach this?

  1. Add a board file overlay in the boards folder? (pinnacle_100_dvk can be used to test the HL7800 driver).
  2. Add a new hl7800.conf that exercises all HL7800 driver options? testcase updated to use hl7800.conf with board pinnacle_100_dvk.

I don't know if we can get away with doing something similar to i2c.dtsi called modem.dtsi and have the hl7800, ublox-sara-r4, and wncm14a2a in there. Than have a modem.conf that enables those drivers (similar to ethernet.conf).

@mike-scott
Copy link
Contributor

Change looks good, and I agree with @galak we should have a build test of this driver.

@github-actions github-actions bot added the area: Tests Issues related to a particular existing or missing test label Oct 3, 2020
@rerickson1
Copy link
Member Author

Sounds good. What are your thoughts on how to approach this?

  1. Add a board file overlay in the boards folder? (pinnacle_100_dvk can be used to test the HL7800 driver).
  2. Add a new hl7800.conf that exercises all HL7800 driver options? testcase updated to use hl7800.conf with board pinnacle_100_dvk.

I don't know if we can get away with doing something similar to i2c.dtsi called modem.dtsi and have the hl7800, ublox-sara-r4, and wncm14a2a in there. Than have a modem.conf that enables those drivers (similar to ethernet.conf).

Due to the driver relying on a UART and multiple IO, couldn't see how to do it like that. Added the test for the HL7800 driver and made use of the pinnacle_100_dvk because it already supports the HL7800.

@rerickson1 rerickson1 requested a review from galak October 3, 2020 00:39
@rerickson1
Copy link
Member Author

Change looks good, and I agree with @galak we should have a build test of this driver.

@mike-scott get I get a re-review, thanks!

jukkar
jukkar approved these changes Oct 8, 2020
View reviewed changes
Copy link
Member

@jukkar jukkar left a comment

Choose a reason for hiding this comment

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

LGTM

@rerickson1
Copy link
Member Author

@galak can I get a second look and get this merged?

galak
galak requested changes Oct 14, 2020
View reviewed changes
Copy link
Collaborator

@galak galak left a comment

Choose a reason for hiding this comment

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

can you rename modem_hl7800.conf to modem.conf

@rerickson1
Copy link
Member Author

can you rename modem_hl7800.conf to modem.conf

are you sure? the settings in there are specific to the hl7800 modem. Different modems may have different settings.

@galak
Copy link
Collaborator

galak commented Oct 14, 2020

are you sure? the settings in there are specific to the hl7800 modem. Different modems may have different settings.

I am, as I can follow up this PR w/having the test build support for multiple modems at the same time.

@galak
Copy link
Collaborator

galak commented Oct 15, 2020

@rerickson1 / @mike-scott here's a PR based on the build_all change in here that enables additional modem support:

#29211

I would like to squash it down to one commit for the build_all changes, and one for the build bug fix, but left it as is for now for review purposes (and signed-off-by ack if I squash it all down) from @rerickson1 .

@rerickson1
tests: drivers: build_all: add HL7800 driver
afbd392 
Add the HL7800 modem driver to the
build_all drivers test.

Signed-off-by: Ryan Erickson <[email protected]>
@rerickson1 rerickson1 requested a review from nashif as a code owner October 15, 2020 10:50
@galak galak merged commit 669ab7b into zephyrproject-rtos:master Oct 15, 2020
@rerickson1 rerickson1 mentioned this pull request Oct 15, 2020
Closed
@rerickson1 rerickson1 deleted the hl7800_fw_update branch April 5, 2021 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mike-scott mike-scott mike-scott approved these changes

@jukkar jukkar jukkar approved these changes

@galak galak galak approved these changes

@rlubos rlubos Awaiting requested review from rlubos

@nashif nashif Awaiting requested review from nashif

Assignees
No one assigned
Labels
area: Modem area: Tests Issues related to a particular existing or missing test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rerickson1 @galak @mike-scott @jukkar