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

move addrList allocation into setNodeID() #221

Open
2bndy5 opened this issue Feb 25, 2023 · 3 comments · May be fixed by #248
Open

move addrList allocation into setNodeID() #221

2bndy5 opened this issue Feb 25, 2023 · 3 comments · May be fixed by #248

Comments

@2bndy5
Copy link
Member

2bndy5 commented Feb 25, 2023

I'm only pursuing this proposal because it would obsolete the private member addrMemAllocated. Initially, this idea came to me (in #220) when discovering that static addresses have to be assigned after calling begin(). In order for a network administrator to give priority to statically assigned addresses, they'd have to assign the addresses before calling update(); this proposal won't change that. However, the requirement for calling begin() feels a bit cumbersome as the addrList array has nothing to do with initializing the required hardware.

I can't think of a reason to keep the allocating code in begin(), so maybe there's a historical reason about why it was written the way it is now.

The proposed code change is pretty straight forward as mentioned in the issue title.

@2bndy5
Copy link
Member Author

2bndy5 commented Feb 25, 2023

I can't think of a reason to keep the allocating code in begin()

I just thought of a reason: Allocating the array is dependent on the ID number passed to setNodeID(). I'm not sure if this proposal is feasible now.

@2bndy5
Copy link
Member Author

2bndy5 commented Feb 25, 2023

Maybe we can move the allocating code into setNodeID(). As for obsoleting the addrMemAllocated member, we might be able to check if the pointer is null instead.

@TMRh20
Copy link
Member

TMRh20 commented Feb 26, 2023

I think those are probably both good ideas.

@2bndy5 2bndy5 changed the title move addrList allocation to c'tor move addrList allocation to seNodeID() Feb 26, 2023
@2bndy5 2bndy5 changed the title move addrList allocation to seNodeID() move addrList allocation into setNodeID() Feb 26, 2023
2bndy5 added a commit that referenced this issue Jun 21, 2024
@2bndy5 2bndy5 linked a pull request Jun 21, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants